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

refactor(menu): update button text (RJE-5) #12

Merged
merged 6 commits into from
May 28, 2020
Merged

Conversation

nmrichards
Copy link
Contributor

No description provided.

@nmrichards nmrichards changed the title refactor(menu): update button text refactor(menu): update button text (RJE-5) May 26, 2020
Copy link
Collaborator

@wallace41290 wallace41290 left a comment

Choose a reason for hiding this comment

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

Since the text is a little longer, do they still fit on all screen sizes? No weird wrapping or overlapping?

Copy link
Collaborator

@wallace41290 wallace41290 left a comment

Choose a reason for hiding this comment

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

Let's rebase this now and try the nested menu items to make it more responsive and remove clutter.

Copy link
Collaborator

@wallace41290 wallace41290 left a comment

Choose a reason for hiding this comment

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

A couple of other things I noticed:

  • When clicking on a nav button, I think it scrolls down too far, sometimes the heading is covered by the app bar. I think we need an offset equal to the height of the app bar?
  • The desktop app bar buttons probably could use some left and right margin of 8px.

src/containers/nav-bar.html Outdated Show resolved Hide resolved
src/shared/menu-util.ts Show resolved Hide resolved
src/containers/nav-bar.html Show resolved Hide resolved
src/containers/nav-bar.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@wallace41290 wallace41290 left a comment

Choose a reason for hiding this comment

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

LGTM

@nmrichards nmrichards merged commit 5a54ceb into master May 28, 2020
@nmrichards nmrichards deleted the refactor/nav branch May 28, 2020 17:08
@wallace41290
Copy link
Collaborator

🎉 This PR is included in version 1.2.1 🎉

The release is available on GitHub release

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.

None yet

2 participants