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

Add tooltips to explain what each mode type indicates #1412

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

tillyw
Copy link
Contributor

@tillyw tillyw commented Mar 22, 2024

Associated issues

fixes cityofaustin/atd-data-tech#15987

Testing

URL to test:
https://deploy-preview-1412--atd-vzv-staging.netlify.app/

Steps to test:

  • click the new info icon to the right of the "By Travel Mode" header and confirm the modal pops up with the correct information!
  • as requested, there should no longer be a fade-in animation on any of the info popover modals ✨ no starwipe i'm sorry ❌ ❇️ 😢

Ship list

  • Check migrations for any conflicts with latest migrations in master branch
  • Confirm Hasura role permissions for necessary access
  • Code reviewed
  • Product manager approved

Copy link
Contributor

@mddilley mddilley left a comment

Choose a reason for hiding this comment

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

Looks great, and I love the new info! ⭐✨ 🚢

@@ -222,7 +224,9 @@ const CrashesByMode = () => {
<Container className="m-0 p-0">
<Row>
<Col>
<h2 className="text-left, font-weight-bold">By Travel Mode</h2>
<h2 className="text-left, font-weight-bold">By Travel Mode{" "}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you removed the comma next to this text-left class since you are in here? That snuck its way in there somewhere in the past.

Also, no change needed, but I noticed we use a space character around the app to create space between elements. Probably something we could have reached to CSS styles to achieve!

Copy link
Member

Choose a reason for hiding this comment

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

thanks for pointing that out, Mike. i would also rather see spacing addressed here with a padding class rather than " ", but i guess we should tackle that separately so that we can fix it everywhere at once?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 - i think that is out of scope too 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, y'all! @mddilley I dropped the comma, thanks for noticing that! and +1 to fixing the spacing using css throughout the app. going to go ahead and merge for the next release !

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

looks great @tillyw!

⭐ ⛵

@@ -39,6 +39,7 @@ const InfoPopover = ({ config }) => {
isOpen={modal}
toggle={toggle}
zIndex={1305} // Set z-index to supercede SideDrawer and SideMapControlDateRange components
fade={false}
Copy link
Member

Choose a reason for hiding this comment

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

thank you!

@@ -222,7 +224,9 @@ const CrashesByMode = () => {
<Container className="m-0 p-0">
<Row>
<Col>
<h2 className="text-left, font-weight-bold">By Travel Mode</h2>
<h2 className="text-left, font-weight-bold">By Travel Mode{" "}
Copy link
Member

Choose a reason for hiding this comment

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

thanks for pointing that out, Mike. i would also rather see spacing addressed here with a padding class rather than " ", but i guess we should tackle that separately so that we can fix it everywhere at once?

@tillyw tillyw merged commit cc8e59c into master Mar 26, 2024
3 of 4 checks passed
@tillyw tillyw deleted the 15987_travel_mode_tooltip branch March 26, 2024 15:43
@johnclary johnclary changed the title Adds tooltip to travel mode component Add tooltips to explain what each mode type indicates Mar 28, 2024
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.

[Enhancement] Add tooltips to explain what each mode type indicates
3 participants