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

Navigation and avatar changes #942

Merged
merged 8 commits into from
Sep 10, 2020
Merged

Navigation and avatar changes #942

merged 8 commits into from
Sep 10, 2020

Conversation

arivepr
Copy link
Contributor

@arivepr arivepr commented Aug 28, 2020

This PR is to move in the changes made for cards 3085 and 8493.
The PF Avatar is now all integrated with the dropdown menu and "Documentation" link has been moved to the bottom of the nav.

ryelo
ryelo previously requested changes Aug 31, 2020
@@ -18,14 +18,14 @@ const openshiftLinks = {
link: 'https://access.redhat.com/support/cases'
},
feedback: {
title: 'Cluster Manager Feedback',
title: 'Cluster Manager Feedback',
Copy link
Member

Choose a reason for hiding this comment

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

Extra space

link: 'mailto:ocm-feedback@redhat.com'
},
marketplace: {
title: 'Red Hat Marketplace',
link: 'https://marketplace.redhat.com'
}
};
};
Copy link
Member

Choose a reason for hiding this comment

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

Extra space

@@ -79,10 +81,12 @@ export class UserToggle extends Component {
render() {
const { isOpen } = this.state;
const { account, isSmall, extraItems } = this.props;
const toggle = isSmall ?
const toggle = isSmall
?
Copy link
Member

Choose a reason for hiding this comment

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

Move this back up to line 84

Comment on lines 98 to 102
.ins-c-dropdown-icon {
height: 36px;
width: 36px;
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to a new file UserIcon.scss and import it in UserIcon.js?

@@ -106,6 +111,7 @@ aside {
.pf-c-page__header-tools .pf-c-avatar {
height: var(--pf-global--FontSize--4xl);
}

Copy link
Member

Choose a reason for hiding this comment

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

extra space

@@ -26,7 +26,7 @@ export class UserIcon extends Component {
}

render() {
const { avatar } = this.state;
const { avatar } = this.state;
Copy link
Member

Choose a reason for hiding this comment

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

Extra space

@ryelo
Copy link
Member

ryelo commented Sep 9, 2020

@arivepr You can run npm run lint to generate these errors, but copying the lint:

/home/travis/build/RedHatInsights/insights-chrome/src/js/App/Header/Tools.js

  14:8  error  'UserIcon' is defined but never used  no-unused-vars

/home/travis/build/RedHatInsights/insights-chrome/src/js/App/Header/UserToggle.js

  8:10  error  'Avatar' is defined but never used  no-unused-vars

LGTM after these are fixed 😄

@@ -1,17 +1,13 @@
import React, { useState, useEffect } from 'react';

import { Button } from '@patternfly/react-core/dist/js/components/Button/Button';
import { Button } from '@patternfly/react-core/dist`/js/components/Button/Button';
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra character

Suggested change
import { Button } from '@patternfly/react-core/dist`/js/components/Button/Button';
import { Button } from '@patternfly/react-core/dist/js/components/Button/Button';

@karelhala
Copy link
Contributor

Remove the extra ` and update snapshots and we are golden!

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #942 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #942   +/-   ##
=======================================
  Coverage   52.58%   52.58%           
=======================================
  Files          53       53           
  Lines        1046     1046           
  Branches      212      212           
=======================================
  Hits          550      550           
  Misses        395      395           
  Partials      101      101           
Impacted Files Coverage Δ
src/js/App/Header/Tools.js 73.33% <ø> (ø)
src/js/App/Header/UserIcon.js 83.33% <ø> (ø)
src/js/App/Header/UserToggle.js 86.66% <ø> (ø)
src/js/App/Sidenav/Navigation.js 87.23% <ø> (ø)

@ryelo ryelo added the on hold label Sep 10, 2020
@karelhala karelhala dismissed ryelo’s stale review September 10, 2020 18:36

Everything looks green, merging!

@karelhala karelhala merged commit 79c79fe into master Sep 10, 2020
@ryelo
Copy link
Member

ryelo commented Sep 10, 2020

@karelhala sorry I added the on hold label because we were tweaking some loading states. We'll throw it in another branch 😄

@ryelo ryelo deleted the NavigationAndAvatarChanges branch September 10, 2020 18:47
@karelhala
Copy link
Contributor

@ryelo too fast on the merge button 😁

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.

4 participants