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

Changelog Modal #440

Merged
merged 14 commits into from
Mar 7, 2024
Merged

Changelog Modal #440

merged 14 commits into from
Mar 7, 2024

Conversation

ptruong0
Copy link
Contributor

@ptruong0 ptruong0 commented Feb 25, 2024

Description

Created a modal element that contains a text description and image. Should only appear once since I set a variable in local storage that prevents the modal from being shown the next time the website is loaded.

Screenshots

image

Steps to verify/test this change:

  • Verify changes work as expected on staging instance

Final Checks:

  • Verify successful deployment

(optional)

  • Write tests
  • Write documentation

Issues

Closes #

@ptruong0 ptruong0 temporarily deployed to staging-440 February 25, 2024 21:47 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 February 25, 2024 22:26 — with GitHub Actions Inactive
@ptruong0
Copy link
Contributor Author

The content can be changed later, does the UI look okay?

@Awesome-E
Copy link
Member

Same thing as I said here for the most part: #426 (comment)

I think it should be consistent with #419's modal with the background color, no borders, and large X button. I believe I also used a filled background with white text for the button, though that was more of a call to action than this one.

Layout other than that looks fine to me.

@ptruong0 ptruong0 self-assigned this Feb 28, 2024
@ptruong0 ptruong0 temporarily deployed to staging-440 February 28, 2024 05:09 — with GitHub Actions Inactive
@ptruong0
Copy link
Contributor Author

Do you think the close button is still necessary? Right now, there are 3 ways to close the modal: (1) X button, (2) Close button at the bottom, (3) Clicking outside the modal.

@js0mmer
Copy link
Member

js0mmer commented Feb 28, 2024

I'd say it's unnecessary.

@ptruong0 ptruong0 temporarily deployed to staging-440 February 28, 2024 05:42 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 February 28, 2024 05:45 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 February 29, 2024 02:17 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 February 29, 2024 02:18 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 February 29, 2024 02:18 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 February 29, 2024 02:38 — with GitHub Actions Inactive
@js0mmer js0mmer self-requested a review February 29, 2024 07:54
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

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

LGTM! I think for now we can just comment out the ChangelogModal within the App component. Once we add something new, someone can uncomment it and write up a changelog.

@Awesome-E
Copy link
Member

The padding of this modal looks different from the padding of the year modal, but I think this is not as big of a deal right now (I should make a PR later to use those styles for all modal elements instead of asking each PR author to copy code to make theirs consistent)

However, I do think you should make the modal enter/exit smoothly like the add year one instead of just disappearing instantly when the close button is clicked

@js0mmer js0mmer linked an issue Mar 1, 2024 that may be closed by this pull request
@ptruong0 ptruong0 temporarily deployed to staging-440 March 5, 2024 22:10 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 March 5, 2024 22:18 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 March 5, 2024 22:21 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 March 6, 2024 04:08 — with GitHub Actions Inactive
@ptruong0 ptruong0 temporarily deployed to staging-440 March 7, 2024 02:52 — with GitHub Actions Inactive
@ptruong0 ptruong0 merged commit 8d3bd82 into master Mar 7, 2024
3 checks passed
@ptruong0 ptruong0 deleted the phil/changelog-modal branch March 7, 2024 02:53
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.

Add changelog modal
3 participants