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.

Sign up for GitHub

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

Jump to bottom

Store: Update header area to remove duplicate site info #23705

Merged
merged 4 commits into from
Apr 20, 2018
Merged

Store: Update header area to remove duplicate site info #23705

merged 4 commits into from
Apr 20, 2018

Conversation

justinshreve
Copy link
Contributor

Fixes #23343, and is a follow-up to #22541.

This PR removes the duplicate site information from displaying on desktop, and implements Jay's design. It also replaces the back arrow with a chevron as requested.

Before:

screen shot 2018-03-27 at 2 28 32 pm

After:

screen shot 2018-03-27 at 2 14 07 pm

To Test:

  • Visit any store page and verify the header bar displays correctly.
  • Click around to a few pages and verify there as well.
  • Double check mobile styles/display.

@justinshreve justinshreve added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR Store labels Mar 27, 2018
@justinshreve justinshreve self-assigned this Mar 27, 2018
@justinshreve justinshreve requested review from jameskoster, kellychoffman and a team March 27, 2018 21:31
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/23343-header

jameskoster
jameskoster reviewed Mar 28, 2018
View reviewed changes
Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

Looks good, but shouldn't the site icon / title be a link?

@kellychoffman
Copy link
Member

Looks good, but shouldn't the site icon / title be a link?

It is on the small screen, which is inline with the site icon on other non-store sections.

:shipit:

@justinshreve
Copy link
Contributor Author

@kellychoffman I just pushed a change that linked them on desktop before seeing your reply. Should I remove the link or keep it?

Note that previously

screen shot 2018-03-27 at 2 28 32 pm

the left most site title was linked on desktop.

@ryelle
Copy link
Member

ryelle commented Mar 28, 2018

Looks good on smaller screens, but at desktop sizes the action buttons are cut off (this is chrome, also happens in safari):

screen shot 2018-03-28 at 3 46 35 pm

I'm also not sure about the chevron in general -- it looks like it should be a back button, but it takes me out of the store section entirely.

screen shot 2018-03-28 at 3 50 39 pm

@justinshreve
Copy link
Contributor Author

@ryelle Thanks for catching that about the buttons.. Should be fixed now.

I don't have a strong preference on the chevron vs arrow. It was requested here to match up with the option that takes you into the section:

ryelle
ryelle reviewed Mar 28, 2018
View reviewed changes
Copy link
Member

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Alignment of the buttons looks good now, just a few comments on the site links

client/extensions/woocommerce/components/action-header/index.js Outdated
@@ -48,6 +49,8 @@ class ActionHeader extends React.Component {
render() {
const { children, primaryLabel, site } = this.props;

const link = getLink( 'https://:site/', site );
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just use site.URL here - just to ensure we have the full URL correct

client/extensions/woocommerce/components/action-header/index.js Outdated
@@ -58,9 +61,15 @@ class ActionHeader extends React.Component {
<Gridicon icon="chevron-left" />
</Button>
<div className="action-header__content">
<SiteIcon site={ site } />
<a href={ link }>
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 add aria-label={ site.title } to the <a />, so that screen readers can have some link text?

client/extensions/woocommerce/components/action-header/index.js Outdated
{ site && <p className="action-header__site-title">{ site.title }</p> }
{ site && (
<p className="action-header__site-title">
<a href={ link }>{ site.title }</a>
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this doesn't need to be a link, it's a very small click/touch target, and causes a duplicate link which can be annoying to screen reader users (assuming the aria-label is added to the icon 🙂 ).

@justinshreve
Copy link
Contributor Author

@ryelle Thanks for the comments. Addressed, and agreed about just linking the site icon.

@justinshreve justinshreve removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Mar 28, 2018
ryelle
ryelle approved these changes Apr 4, 2018
View reviewed changes
Copy link
Member

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Tested again after the updates, looks good!

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 5, 2018
@gwwar
Copy link
Contributor

gwwar commented Apr 17, 2018

@justinshreve any reason why this wasn't merged yet? Did this need another look?

@timmyc
Copy link
Contributor

timmyc commented Apr 18, 2018

why this wasn't merged yet? Did this need another look?

@gwwar Justin has been afk amidst a big move. I'll rebase this and get it shipped today.

@timmyc timmyc force-pushed the fix/23343-header branch from e928224 to ffe13e3 Compare April 19, 2018 23:39
@timmyc timmyc merged commit 2f7c752 into master Apr 20, 2018
@justinshreve justinshreve deleted the fix/23343-header branch May 11, 2018 13:09
@justinshreve justinshreve restored the fix/23343-header branch May 11, 2018 13:09
@justinshreve justinshreve deleted the fix/23343-header branch May 11, 2018 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameskoster jameskoster jameskoster left review comments

@kellychoffman kellychoffman kellychoffman approved these changes

@ryelle ryelle ryelle approved these changes

Assignees

@justinshreve justinshreve

Labels
Store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@justinshreve @matticbot @kellychoffman @ryelle @gwwar @timmyc @jameskoster