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

UI V2: header #696

Merged
merged 21 commits into from
Dec 3, 2020
Merged

UI V2: header #696

merged 21 commits into from
Dec 3, 2020

Conversation

scarlettperry
Copy link
Contributor

@scarlettperry scarlettperry commented Nov 19, 2020

Description

This PR

  • updates the Header, Logo, and UserInformation components
  • adds the notification icon (created a Notifications component for when we implement that feature)
  • adds a Header, Notifications, and UserInformation story

Testing Performed

Created the components in storybook
Ran make frontend-lint and make frontend-test without errors

Before
Screen Shot 2020-11-19 at 3 30 34 PM

New (reflects latest commits)
Screen Shot 2020-12-02 at 7 16 35 PM

Untitled 5

@scarlettperry scarlettperry changed the title add v2 design for header UI V2: header Nov 19, 2020
@scarlettperry
Copy link
Contributor Author

scarlettperry commented Nov 19, 2020

hi @dschaller, mind giving this a first pass review? Still need to see what snapshots to update. Looks like this is the only related snapshot

exports[`AppLayout component renders correctly 1`] = `

@scarlettperry scarlettperry marked this pull request as ready for review November 20, 2020 00:04
@scarlettperry scarlettperry requested a review from a team as a code owner November 20, 2020 00:04
@scarlettperry scarlettperry marked this pull request as draft November 20, 2020 20:54
@scarlettperry scarlettperry marked this pull request as ready for review November 30, 2020 18:16
Copy link
Collaborator

@danielhochman danielhochman left a comment

Choose a reason for hiding this comment

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

quick pass, looking good

@dschaller
Copy link
Contributor

Overall looks really good! A few more comments here and we should be good to go. Let's get this merged into the feature branch and open follow up PRs to drill into what the notification and user menus should look like with content populated. Feel free to punt on things like the hover and active states until then as well.

Copy link
Collaborator

@danielhochman danielhochman left a comment

Choose a reason for hiding this comment

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

tiny nits, lgtm otherwise

we will need a way to disable the notifications functionality via a prop since we don't support it yet, but we can follow up with that later.

@@ -68,20 +56,22 @@ const Header: React.FC = () => {

return (
<>
<AppBar position="static" color="secondary">
<AppBar position="static" color="secondary" elevation={0}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

has no effect with CSS overrides

Suggested change
<AppBar position="static" color="secondary" elevation={0}>
<AppBar position="static" elevation={0}>

Comment on lines 29 to 30
color: "#ffffff",
opacity: "0.87",
Copy link
Collaborator

Choose a reason for hiding this comment

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

make opacity part of the font color

Suggested change
color: "#ffffff",
opacity: "0.87",
color: "rgba(255, 255, 255, 0.87)",

@scarlettperry scarlettperry merged commit 73cc79d into UIV2 Dec 3, 2020
@scarlettperry scarlettperry deleted the sperry-header-uiv2 branch December 3, 2020 19:27
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