Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Responsive charts #11

Merged
merged 16 commits into from
Oct 9, 2024
Merged

Responsive charts #11

merged 16 commits into from
Oct 9, 2024

Conversation

ddanieltan
Copy link
Contributor

#9

This PR adds the full_width parameter to altair2fasthtml and custom CSS for responsive charts.

The full_width parameter is optional, and defaults to False. When set to True, the resulting chart will take up the full width of its parent container.


"""
if full_width:
chart = chart.properties(width="container")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to check, does this override existing properties on the chart?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, if the height is set beforehand, does this test remove that property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I just went to do a little test and confirmed that the original height is preserved.

def generate_chart():
    pltr = pd.DataFrame({"y": [1, 2, 3, 2], "x": [3, 1, 2, 4]})
    chart = (
        alt.Chart(pltr)
        .mark_line()
        .encode(x="x", y="y")
        .properties(width=400, height=200)
    )
    return altair2fasthtml(chart, full_width=True)

x = generate_chart()

# Results in a chart with height = 200, width = "container"
print(to_xml(x))

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, could we add that as a unit test?

@koaning
Copy link
Owner

koaning commented Oct 9, 2024

Sweet. LGTM. I will make a release as well.

One thing before I do, you've done some good work here by not only suggesting the improvement but also implementing it so I would like to give you a shoutout on my socials. Got a Twitter/LinkedIn account? If you prefer not to get the extra attention feel free to say so (some folks prefer that).

@koaning koaning merged commit acf0330 into koaning:main Oct 9, 2024
1 check passed
@ddanieltan
Copy link
Contributor Author

Thanks Vincent, that's very nice of you to suggest! Your feedback and suggestions made for a great collaborative experience for me too. I'll DM you my socials on Discord since I see you in the FastHTML server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants