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

committee-project-gallery | building a universal popup to display on all projects #45

Closed
wants to merge 5 commits into from

Conversation

itsoliviasparks
Copy link
Collaborator

Developing a universal popup to display on all TCL projects on the upcoming gallery page on the-collab-lab.codes
This has been an ongoing effort over the last few months (along with @segdeha, @djtaylor8, and @aayushkh)

Hi Team,
Wanted to open a PR for this one to get everyones feedback before we add this to all projects.

I've developed a Popup component with styling (matching TCL site ✨) that is displayed to the user when they first get to the site.

I wanted to change as little of each project's code as possible to make this easy to implement regardless of how each team coded/styled their app. (Which is why I went with useState as a toggle rather than useEffect, but I'm open to switching or tackling another way. I've been outta react for a minute 😅)

I've been able to implement this on TCL-61, TCL-58, & TCL-55 with no issue! 💪

For the copy, I borrowed a lot from the TCL site, and wrote a list of what is/what isn't working.
@segdeha I would particularly love to hear your thoughts here!

Screenshot 2023-12-08 at 17 25 09

Copy link

github-actions bot commented Dec 9, 2023

Visit the preview URL for this PR (updated for commit 63dc2b8):

https://tcl-61-smart-shopping-list--pr45-popup-94chglgy.web.app

(expires Sun, 17 Dec 2023 00:15:06 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c53baec83435703c923d25f6be479829f5c47562

@segdeha
Copy link
Member

segdeha commented Dec 9, 2023

This looks amazing, @itsoliviasparks, nice work!!!

Of course, I do have questions 😅 :

  • How does this look on mobile? I assume it's kind of a full-screen takeover on a phone. Does the content easily scroll?
  • The content of the modal is really thoughtful and complete! I almost wonder if you could take out that long paragraph towards the end to save space?
  • This is a nitpick, but I'd change that sentence about TCL to be: "The Collab Lab provides collaborative, remote blah blah blah."
  • I think we have a namespace in npm, is that right? Could we publish the component there so we can easily pull it in to all of the projects from a common place?
  • Lastly, how well does this work with screen readers? What I'd expect as good behavior is that the modal/takeover has focus at first. When it's dismissed, focus goes to the top element in the app itself (whatever would have been focused if the modal wasn't there).

I'm going to default to approve on the PR to keep you unblocked. I trust your (& the rest of the team's) judgement to implement something that works well!

Copy link
Member

@segdeha segdeha left a comment

Choose a reason for hiding this comment

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

Love it! See my comment for some questions, but otherwise this looks great!

Copy link

@djtaylor8 djtaylor8 left a comment

Choose a reason for hiding this comment

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

This looks amazing! Great work on this this!

@itsoliviasparks
Copy link
Collaborator Author

Great questions here @segdeha!!

This looks amazing, @itsoliviasparks, nice work!!!

Of course, I do have questions 😅 :

  • How does this look on mobile? I assume it's kind of a full-screen takeover on a phone. Does the content easily scroll?

I haven't looked into this on mobile yet, but that's next on my to-dos now that I have everyone's buy-in!

  • The content of the modal is really thoughtful and complete! I almost wonder if you could take out that long paragraph towards the end to save space?

For sure!

  • This is a nitpick, but I'd change that sentence about TCL to be: "The Collab Lab provides collaborative, remote blah blah blah."

  • I think we have a namespace in npm, is that right? Could we publish the component there so we can easily pull it in to all of the projects from a common place?

I have never done that, but happy to! I can have a look into it

  • Lastly, how well does this work with screen readers? What I'd expect as good behavior is that the modal/takeover has focus at first. When it's dismissed, focus goes to the top element in the app itself (whatever would have been focused if the modal wasn't there).

That's what I'm expecting too, but I can be sure to test it with a screen reader too!

I'm going to default to approve on the PR to keep you unblocked. I trust your (& the rest of the team's) judgement to implement something that works well!

🙏

@itsoliviasparks
Copy link
Collaborator Author

This component has been renamed to ArchivalNoticeModal & has moved to the shopping-list-utils repo. It will be added to each project through npm.

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.

3 participants