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

Components: header cake cleanup #1514

Merged
merged 3 commits into from
Jan 29, 2016
Merged

Conversation

adambbecker
Copy link
Contributor

Cleans up the "header cake" (client/components/header-cake) component & fixes #143.

Background

Over in #143, a situation arose with the current header cake component where the "back" text would wrap oddly on small screens if displayed in certain languages. After some investigation, it turns out that this situation could occur in a variety of situations as long as the text was long enough.

There was a variety of potential tweaks that were investigated (#143) and could be made in order to handle the situation. This PR proposes one of them, which aligns the title to the right of the card and allows the "back" text to always remain inline with it's accompanying chevron icon.

Updated: 12/14/15

Changes summary

This PR proposes a solution arrived at via the discussion on #143 and comments on this PR from an earlier version of it. The overall goal (visually) is to center the title text while keeping as much of the "back" text visible as possible above 480px. Below 480px, the "back" text will be hidden (note: chevron still visible) if it is 8 characters or above.

Component structure

The component's structure is changed slightly with the introduction of a HeaderCakeBack sub-component whose job is just to display the "back" text/link. That sub-component also is the one that keeps track of window width and text length in order to decide if it should render just the chevron or the chevron + text.

Visual changes

Visually, the component shouldn't be all that different from what it currently looks like (intentionally), but should be better at displaying as much of both the title and the back text as possible. This is because of a shift in the CSS where the "corners" of the header cake are no longer tied to a distributed width, but will only take up as much space as they need (with a maximum of 1/3 the container width) to display the back text which allows the title to fill all the remaining space.

The main notable visual change could occur if the back sub-component's text is over 8 characters and the window's width is below 480px. This currently occurs in some language's translations of the word "back" as well as two other places in english: the devdocs/design sub-pages & in the editor's media modals' detail screens.

Examples

Domain settings

/devdocs/design/gridicons

screen shot 2015-12-14 at 1 23 46 pm

screen shot 2015-12-14 at 1 24 56 pm

Devdocs sub-page

/devdocs/design/gridicons

screen shot 2015-12-14 at 1 24 10 pm

screen shot 2015-12-14 at 1 24 20 pm

Note: "back" text does away below 480px because the string all components is over 8 characters

Delete site

/settings/delete-site/*

screen shot 2015-12-14 at 4 34 31 pm

screen shot 2015-12-14 at 4 34 42 pm

Editor media modal

/post/* (media modal detail)

screen shot 2015-12-14 at 4 28 00 pm

screen shot 2015-12-14 at 4 28 13 pm

Effected implementors

The editor media modal header cake (client/post-editor/media-modal/detail) was the only component that initially required some tweaking, but upon further investigation, a slight refactor of the css allowed for a little cleaner implementation of the header cake itself.

Currently, the media modal header cake does some overriding css in order to achieve it's intended display method. With the changes to the header cake component in this PR, the media modal implementation was tweaked via cae152c so that the input no longer relies on absolute positioning in order to achieve the desired effect.

Testing

Visit any page that contains the HeaderCake component and check all the text (title & back link) will no longer wrap at certain sizes. It will try to display as much text as possible, but might become ellipsis'd in certain situations.

If the "back" text is 8 characters or above and the screen size is below 480px, only the chevron should be visible.


/cc @mtias, @hoverduck

@adambbecker adambbecker 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 Components labels Dec 11, 2015
@adambbecker adambbecker self-assigned this Dec 11, 2015
@adambbecker adambbecker added this to the Core: Iteration 17 milestone Dec 11, 2015
@sirbrillig
Copy link
Member

FWIW it looks a little awkward in the Verticals Survey component:

screen shot 2015-12-11 at 5 12 23 pm

Maybe there could be an option to center the text?

@folletto
Copy link
Contributor

Thanks for tackling this. :)

I find the right alignment however a bit odd, doesn't feel like a title in general, and more specifically consolidated mobile patterns have action items on that kind of UI element, so I feel that they could be perceived as action (iOS specifically, but Android has icons there too).

This is at the same time a very very important navigation component and a hard one. It's very important because it's what allows our sections to drill-down without complicating further the top level structure, but the bar itself is strange because in practice the title is there just to balance the white on the side of "Back", otherwise could really be removed.

My take here would be:

  1. For the scope of this PR, let's keep the title centered.
  2. Let's see if we can find some way to solve that "Back" issue more elegantly, if possible.

@adambbecker
Copy link
Contributor Author

@sirbrillig & @folletto: Thanks for the feedback! I had a feeling the right aligned text might just "not look right" and apparently it does.

For other solutions as you've both mentioned, I've outlined a few options we can take over in #143. Basically what it comes down to is that the right aligned option provides the most amount of visible text that doesn't wrap. There are also a variety of other solutions mentioned in that issue (#143), and as with most things on the web, they all come with trade-offs.

In the meantime, I'll re-center the title and ellipsis everything else. The results of which have examples in #143.

@adambbecker adambbecker added [Status] In Progress and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 14, 2015
@folletto
Copy link
Contributor

(#143) ... 2. Ellipsis "back" text

I think we should consider something along these lines, even dropping text entirely on smaller sizes (if there was a nice way to do that with different text lengths would be amazing).

In the end, just the back arrow is a consolidated behaviour, even more on mobile, so I don't think that losing the text would be a problem. :)

@adambbecker
Copy link
Contributor Author

In the end, just the back arrow is a consolidated behaviour, even more on mobile, so I don't think that losing the text would be a problem. :)

Ooh, this is a good idea and I agree. I'll check that out.

@adambbecker
Copy link
Contributor Author

@sirbrillig, @mtias, @folletto: Alrighty then, thanks for the all feedback and suggestions. I've reworked the component in order to achieve the centered title, but also displaying as much of the "back" text as possible. It also takes up @folletto's suggestion and will not show the "back" text if it is above 8 characters and the screen size is below 480px. I've gone ahead and updated this PR's description with an explanation of all the updates.

Also note for @mtias & @folletto that I did include a slight rework of the client/post-editor/media-modal/detail component's css which removes the display overrides and absolute positioning tweaks. Visually it should behave very similarly as what's currently in production, just without having to rely on the positioning tweaks.

@adambbecker adambbecker added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 14, 2015
@folletto
Copy link
Contributor

Tested on the instances above.
Design wise, 👍

@sirbrillig
Copy link
Member

Tested on Verticals. 👍

mixins: [ ObserveWindowSizeMixin ],

propTypes: {
onClick: PropTypes.func,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this optional? If not we can add a PropTypes.func.isRequired here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this particular case it is actually optional because of the spacer usage. In order to give the title the most about of space possible and still keep in centered, the corners were changed to display the same text in both places (the right one is hidden). The right corner still uses the back component, but doesn't include any onClick props.

@gwwar
Copy link
Contributor

gwwar commented Dec 16, 2015

Code looks good 👍

I found a minor issue with the loading placeholders. (If this is expected, feel free to 🚢 )
loading

@adambbecker adambbecker force-pushed the fix/143-header-cake-back-overflow branch 2 times, most recently from 54da62a to cb45267 Compare December 17, 2015 17:36
@adambbecker
Copy link
Contributor Author

@gwwar: Thanks for all the feedback and taking a look!

I know you're currently occupied elsewhere (have fun btw!), just wanted to update this thread that I've updated that domain management placeholder to be more visually appropriate.

screen shot 2015-12-17 at 1 06 29 pm

/cc @breezyskies


Overall, I think we're looking pretty good here. I'll try to get one more quick review just to confirm and we'll get this out.


getDefaultProps() {
return {
text: 'Back'
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to translate 😄

getDefaultProps() {
    return { text: this.translate( 'Back' ) };
}

@dmsnell
Copy link
Member

dmsnell commented Dec 28, 2015

@adambbecker nice looking code! I left a few comments and questions - not sure I understand all the context for this though so if they seem off-base that's my fault.

@dmsnell dmsnell added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 28, 2015
@artpi
Copy link
Contributor

artpi commented Dec 28, 2015

Is it supposed to be like this?

http://calypso.localhost:3000/domains/manage/piszek.com/edit/piszek.com

@adambbecker adambbecker force-pushed the fix/143-header-cake-back-overflow branch from cb45267 to bca16eb Compare January 4, 2016 17:54
@adambbecker
Copy link
Contributor Author

@dmsnell: Cool, thanks for taking a look! I took care of most of your suggestions and your context was spot on, all good stuff. Especially like the defaults for destructuring, very helpful to know!

@artpi: It is supposed to look like that to a point. It is indeed supposed to ellipsis certain texts and the component will hide the "back" text if both these requirements are met:

  • "back" text character length is over 8 characters
  • window width is less than 480px

That being said, your screenshot highlighted a rather interesting situation where a screen is 275px wide. I don't know of any devices around that size, but I did alter the "back" text hiding condition nonetheless to take care of it and it will now hide the text if the window is under 300px no matter what.

Thanks for the feedback!

@adambbecker adambbecker added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jan 4, 2016
@apeatling
Copy link
Member

@adambbecker I think we can merge these changes now?

@gwwar
Copy link
Contributor

gwwar commented Jan 26, 2016

I retested this and it looks good! :shipit:

@gwwar gwwar 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 Jan 26, 2016
Cleans up "header cake" component and closes #143
Visually tweaks the domain detail management header cake while in placeholder state to be smaller and more inline.
Removes continaer corner div, using just back link instead. Also removed back text if window is below 300px.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Components: Header Cake: layout issues when Back is a longer string
8 participants