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

Fix shrunk icon size in st.expander #7596

Merged
merged 10 commits into from Jan 5, 2024

Conversation

matiboux
Copy link
Contributor

@matiboux matiboux commented Oct 24, 2023

Describe your changes

This PR fixes the expand icon size in st.expander.

Currently, the icon size is shrunk when the label is a long text. See the current behavior:
Screenshot

This PR adds a change to set the flex-shrink: 0 CSS property so that all icons look like the first one, even with large text labels.


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@matiboux matiboux changed the title Add flexShrink to StyledIcon Fix shrunk icon size in st.expander Oct 24, 2023
Copy link

stale bot commented Nov 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 7, 2023
@vdonato vdonato removed the stale label Nov 7, 2023
@vdonato vdonato self-assigned this Nov 7, 2023
Copy link

stale bot commented Dec 7, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@kmcgrady
Copy link
Collaborator

Hey @matiboux Thanks for the contribution and sorry for the delay!

Looks like there's some snapshot errors associated with our metric, which uses the icon. I'll get the snapshots updated as they probably look most correct.

I'd also like to get an end to end test for the expander to ensure this does not pop up again.

@matiboux
Copy link
Contributor Author

Hey @matiboux Thanks for the contribution and sorry for the delay!

Looks like there's some snapshot errors associated with our metric, which uses the icon. I'll get the snapshots updated as they probably look most correct.

I'd also like to get an end to end test for the expander to ensure this does not pop up again.

Thanks! No problem for the delay, I'm happy this is moving forward.
Let me know if I can help in some way.


Doing some research first:

I found resources to help me understand how tests and snapshots can be updated:

Comparing the first metric test snapshot from the CI workflow with e2e_playwright/__snapshots__/linux/st_metric_test/metric-help-and-ellipses[dark_theme-chromium].png, I generated this diff below. It shows the new behavior: the icon is larger, as it does not shrink and it fills the height space, and so the text is shifted to the right.
diffchecker-output

@kmcgrady
Copy link
Collaborator

Hey @matiboux.

Yes the easiest way would be to remove the problematic images, push them, let the CI fail, download the correct artifacts, and push them.

For the expander. We migrated the tests to use playwright. We are in the process of changing our end to end testing since playwright since we can do screenshot tests in multiple browsers (and it's much faster). You can add an example of an expander with a long amount of text here. https://github.com/streamlit/streamlit/blob/develop/e2e_playwright/st_expander.py You can then add a test to assert the screenshot of expander. https://github.com/streamlit/streamlit/blob/develop/e2e_playwright/st_expander_test.py Like cypress it will fail cause there are missing screenshots, you can then add them (making sure they look right), and then the tests should pass.

Let me know if there's any issues. Really appreciate your help!

@matiboux
Copy link
Contributor Author

So I've finally made the changes.

  • I updated test images with those from playwright_test_results/snapshot-updates/linux/st_metric_test.
  • I added two tests on "long expanders", with some loreum ipsum text to make long lines.

I tried not to impact existing tests when I added long expanders tests at the end of the existing test page. The existing test screenshot gets cut at the end, so I'm betting what I added below that won't appear either. I added a new test that generates two new screenshots: one for an expanded long expander, and another for a collapsed long expander.

@matiboux
Copy link
Contributor Author

Hey @kmcgrady,

Please review my changes and tell me if you want me to change something else before we can continue.

If all is good, the workflow requires your approval for running. 🙏
See the "Playwright E2E Tests" workflow run status as "Action required" here: https://github.com/streamlit/streamlit/actions/runs/7315429836.

Copy link
Contributor Author

@matiboux matiboux left a comment

Choose a reason for hiding this comment

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

Reviewing the CI results.

e2e_playwright/st_expander.py Outdated Show resolved Hide resolved
@matiboux
Copy link
Contributor Author

Thank you for the approved workflows run.

CI results reviewed and corrections applied.

js-tests.yml / js-test CI job:

  • Fixed the lint error: missing trailing comma.

playwright.yml CI workflow:

  • Fixed the count assertions for expanders in the main container: I added 2 more, so the existing tests were affected.
  • Fixed the nth values for stExpander-long assertions: I counted the empty expander, but it is actually not rendered.

Finally, despite my wrong nth values, the playwright.yml CI workflow ran partially for my new tests. So I have added & renamed the generated test images, which should work for the stExpander-long-collapsed tests.

I'll need a new workflows run to add the missing stExpander-long-expanded test images.

I'll let you review new changes and approve the workflows run if all seems good.

@matiboux
Copy link
Contributor Author

matiboux commented Jan 5, 2024

@kmcgrady Thank you again.

Latest commit adds the missing stExpander-long-expanded test images, and fixes a mistake of misplacing stExpander-long-collapsed test images. Then, I synced the branch with upstream.

All checks are now successful! ✅
Ready for your review.

@kmcgrady
Copy link
Collaborator

kmcgrady commented Jan 5, 2024

Awesome! Thanks @matiboux! And Happy New Year! Things are looking good here! Thank you so much for your time and patience!

If you're happy, I can get this merged today!

@matiboux
Copy link
Contributor Author

matiboux commented Jan 5, 2024

@kmcgrady All good for me as well!
Happy new year to you too! 🎉
Let's merge! 🚀

@kmcgrady kmcgrady merged commit d428d91 into streamlit:develop Jan 5, 2024
43 checks passed
@matiboux matiboux deleted the fix-st-expander branch January 5, 2024 10:11
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
* Add flexShrink to StyledIcon

* Fix the st_metric_test playwright e2e test images

* Add test for long expanders

* Add trailing comma to fix coding style

* Fix main_expanders count assertions

* Fix nth values for stExpander-long assertions

* Add stExpander-long-collapsed playwright e2e test images

* Add missing & move stExpander-long playwright e2e test images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants