Skip to content

Update modals #6688

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.

Sign up for GitHub

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

Jump to bottom
Merged
mayagbarnes merged 52 commits into from
May 23, 2023
Merged

Update modals #6688

mayagbarnes merged 52 commits into from
May 23, 2023

Conversation

@sfc-gh-jgarcia
Copy link
Contributor

@sfc-gh-jgarcia sfc-gh-jgarcia commented May 15, 2023 •
edited
Loading

📚 Context

This PR updates the look of our modal windows, per @jrieke's request.

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe: Design change

🧠 Description of Changes

  • Removed borders and lines throughout the UI;

  • Removed repetitive buttons;

  • Tighten up spacing between elements, updated paddings and corner radius.

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Screenshot 2023-05-15 at 2 09 02 PM

Screenshot 2023-05-15 at 2 09 57 PM

Screenshot 2023-05-15 at 2 11 18 PM

Screenshot 2023-05-15 at 2 06 52 PM

Screenshot 2023-05-15 at 2 07 26 PM

Screenshot 2023-05-15 at 2 07 39 PM

Screenshot 2023-05-15 at 2 07 45 PM

Screenshot 2023-05-15 at 2 08 13 PM

Screenshot 2023-05-15 at 2 08 28 PM

Screenshot 2023-05-15 at 2 08 37 PM

Current:

Screenshot 2023-05-02 at 11 59 1

Screenshot 2023-05-02 at 11 44 1

Screenshot 2023-05-02 at 11 43 1

Screenshot 2023-05-02 at 11 42 1

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Contribution License Agreement

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

@sfc-gh-jgarcia sfc-gh-jgarcia added the security-assessment-completed Security assessment has been completed for PR label May 15, 2023
@jrieke
Copy link
Collaborator

jrieke commented May 16, 2023

@sfc-gh-jgarcia small nit: we should probably change the title of the clear cache dialog to "Clear cache" (i.e. second word lowercase) to be more inline with the other modals.

@jrieke
Copy link
Collaborator

jrieke commented May 16, 2023 •
edited
Loading

And can you maybe change the title of the about dialog to "Made with" instead of "Powered by"? Wanted to do this for a long time but always forgot. Doesn't have to go into this PR but I assume it's a tiny change so might be worth just adding right away.

@sfc-gh-jgarcia sfc-gh-jgarcia marked this pull request as ready for review May 16, 2023 16:21
mayagbarnes
mayagbarnes reviewed May 16, 2023
View reviewed changes
@sfc-gh-jgarcia sfc-gh-jgarcia changed the title Update modals WIP: Update modals May 17, 2023
@sfc-gh-jgarcia
Copy link
Contributor Author

sfc-gh-jgarcia commented May 18, 2023

Since there are reasonably significant visual changes, I would like to see at least e2e snapshot testing coverage of these different dialogues (to the best of our ability).

@mayagbarnes Sorry for the messy PR, but I had all sorts of issues trying to get to the modals 😅 seems like our matchThemedSnapshots command isn't working becase force is set to false on it, resulting on the cy.click() failed because this element: <button>...</button>i is being covered by another element: <div>...</div> error. So, I ended up using cy.changeTheme and matchImageSnapshot for them. Hopefully that's ok!

Besides that, I also couldn't get to the Edit theme modal, I'm assuming because It's already inside another modal, nor the Clear cache one, under the Developer options. Any pointers on how to approach those would be really helpful, provided that we want all modals to be covered with tests.

Thanks 🙏

@sfc-gh-jgarcia sfc-gh-jgarcia marked this pull request as ready for review May 18, 2023 19:38
@mayagbarnes mayagbarnes changed the title WIP: Update modals Update modals May 21, 2023
mayagbarnes
mayagbarnes reviewed May 21, 2023
View reviewed changes
@mayagbarnes
Copy link
Collaborator

mayagbarnes commented May 21, 2023 •
edited
Loading

@mayagbarnes Sorry for the messy PR, but I had all sorts of issues trying to get to the modals 😅 seems like our matchThemedSnapshots command isn't working becase force is set to false on it, resulting on the cy.click() failed because this element: <button>...</button>i is being covered by another element: <div>...</div> error. So, I ended up using cy.changeTheme and matchImageSnapshot for them. Hopefully that's ok!

Besides that, I also couldn't get to the Edit theme modal, I'm assuming because It's already inside another modal, nor the Clear cache one, under the Developer options. Any pointers on how to approach those would be really helpful, provided that we want all modals to be covered with tests.

Thanks 🙏

@sfc-gh-jgarcia No worries, thanks so much for all your work to add more comprehensive modal testing! Totally works to use cy.changeTheme and matchImageSnapshot in these one-off cases.

Hope its okay but I worked on the Edit theme and Clear cache modal tests in this commit and their resulting snaps in this commit if you want to review and confirm they look the way they should!

mayagbarnes
mayagbarnes approved these changes May 21, 2023
View reviewed changes
Copy link
Collaborator

@mayagbarnes mayagbarnes left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🚀

@sfc-gh-jgarcia
Copy link
Contributor Author

sfc-gh-jgarcia commented May 22, 2023 •
edited
Loading

Thanks a lot for adding those missing tests, @mayagbarnes! Snapshots look good to me, so we should be good to merge these. Thanks, as usual! 🙏

@mayagbarnes mayagbarnes merged commit 794852d into develop May 23, 2023
@mayagbarnes mayagbarnes deleted the update-modals branch May 23, 2023 17:36
tconkling added a commit to tconkling/streamlit that referenced this pull request May 30, 2023
@tconkling
Merge branch 'develop' into tim/DevRequirementsNoProtobuf
31a39c0 
* develop: (22 commits)
  enzyme -> react-testing-library (Countdown, Modal, ProgressBar) (streamlit#6744)
  Remove console.log (streamlit#6753)
  Update `st.data_editor` and `st.dataframe` docstrings (streamlit#6752)
  Update `st.data_editor` session state format (streamlit#6711)
  Cypress flaky test fixes (streamlit#6743)
  Document integer size limit for number_input and slider (streamlit#6724)
  Change default theme hash on app init (streamlit#6729)
  Use .genericFonts instead of .fonts to fix bug with theme postMessage. (streamlit#6732)
  Improve startup performance by lazy loading some dependencies (streamlit#6531)
  Add support for Altair 5 (streamlit#6618)
  Update modals (streamlit#6688)
  Improve docstrings for `ttl` and `max_entries` (streamlit#6733)
  Improve `st.columns` docstring (streamlit#6727)
  Removed orphan line in dataframe docstring (streamlit#6734)
  Fix useIsOverflowing dependency array (streamlit#6731)
  Remove experimental from data editor (streamlit#6712)
  Clarify set_page_config docstring and exception message (streamlit#6594)
  Add a config option to disable warning for setting both a widget default and its key in session_state (streamlit#6640)
  Add column configuration API for `st.dataframe` and `st.data_editor` (streamlit#6598)
  Migrate datetime column formatting from date-fns to momentJS (streamlit#6702)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mayagbarnes mayagbarnes mayagbarnes approved these changes

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sfc-gh-jgarcia @jrieke @mayagbarnes