Skip to content

Conversation

@robertgregg3
Copy link

@robertgregg3 robertgregg3 commented Aug 27, 2021

Blocked by #435 which provides internationalization for the app menu.

Refactored the current menu located in app-menu.js into it's own component with the following uses:

  • Allow sub menus
  • Allow sub menus within submenus
  • optional icons from url or types
  • hyperlink option

Points to note:

  • The code uses the same setup as the patternRow.js expandable mechanics.
  • The menu is currently set up to go 2 sub menus deep: Top level > sub menu > sub menu
  • The additional menu items will sit below the standard menu items
  • The associated PR for the config repo can be found here

WIP notes:

  • I added the css to the index.css file which I can put into it's own file.
  • The hyperlinks for ATL RIDES are not in place and need to be added
  • The icons are just a first draft, mostly hyperlinks. Would need a review for sure
  • Any other improvements or things I have done wrong in the code etc I would be happy to hear

Robert Gregg added 6 commits August 20, 2021 14:55
On Desktop, pressing the hamburger menu on the top left opens a sliding pane which contains the new
app menu. The app menu n ow includes a 'Traveler Tools' menu. This is done by creating a custom
MenuItem component.
@robertgregg3
Copy link
Author

Have just seen that the tests have failed. Will take a look when I am back on Tuesday 31st August

@miles-grant-ibigroup miles-grant-ibigroup self-assigned this Aug 30, 2021
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Foundation is solid, just a few small polishing changes! I think the biggest thing is adjusting the spacing between items and text positions to establish a clear hierarchy of items and make it clearer where submenus begin and end! If you'd me to make some mockups to better explain what I mean please let me know.

Screen Shot 2021-08-30 at 10 00 39 AM

@robertgregg3 robertgregg3 requested review from miles-grant-ibigroup and removed request for landonreed September 9, 2021 17:13
@robertgregg3
Copy link
Author

It is failing on a test from the stop-viewer.js file which I think must be there because of the recent changes you did @binh-dam-ibigroup to the linting etc? I did not touch that file with this PR.

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Really coming along! I'm loving the look (just a small personal opinion about padding). Just a few small cleanup suggestions.

@robertgregg3 robertgregg3 removed their assignment Sep 10, 2021
@miles-grant-ibigroup
Copy link
Collaborator

Thanks @binh-dam-ibigroup for the localization PR! Everything is merged and working. I am going to finish localizing this PR tomorrow morning and then will assign it back to you.

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

A few minor changes needed to close some gaps and it will be good to go.

@miles-grant-ibigroup
Copy link
Collaborator

Good catches, thanks!

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

One tiny last change, but this looks good!

Copy link
Contributor

@philip-cline philip-cline left a comment

Choose a reason for hiding this comment

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

Fixed one tiny typo, everything else looks good to me

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2021

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants