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

SectionHeader: Update default font size #2864

Merged
merged 4 commits into from Feb 2, 2016

Conversation

alternatekev
Copy link
Contributor

Currently, the SectionHeaders use a pretty small font size by default:

screen shot 2016-01-27 at 3 21 39 pm

screen shot 2016-01-27 at 3 21 56 pm

We discussed the possible improvement to the font size of the SectionHeader's default label (I used 14px here, 15px seemed too big), and adding a way to set a compactHeader prop to get the previous 11px sizing. This PR handles that, which looks like this:

screen shot 2016-01-27 at 3 19 30 pm

screen shot 2016-01-27 at 3 24 18 pm

screen shot 2016-01-27 at 3 24 27 pm

With the combination of #2863, Stats would change to look like:

screen shot 2016-01-27 at 2 58 55 pm

cc @rickybanister @folletto @mtias

@alternatekev
Copy link
Contributor Author

I can add a commit here that changes all of the current SectionHeaders to use the compactHeader prop but I wanted to get a feel for if this was going to work before I went and did that. The screenshots above reflect would could happen, not 100% would should happen.

@alternatekev alternatekev 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 labels Jan 27, 2016
@rickybanister
Copy link

I wouldn't mind us moving to the 14px non-small-caps for all section headers. It's easier to read and doesn't change the height of the box at all.

14px is the size of our nav items in SectionNav as well—it would be good to move toward more standardization of both components. Some day SectionNav's height might not jump three times in as many breakpoints.

@folletto
Copy link
Contributor

I felt that using caps there worked very well in separating the title from the content. I'm not overly attached to that idea, but I still feel that caps would work better. :)

Size wise, I think the change is good. :)

@mtias
Copy link
Member

mtias commented Jan 28, 2016

My vote is that whatever we do we stick to one.

@rickybanister
Copy link

No prop, let's just go with 14px non-caps! Merge it quick.

@alternatekev
Copy link
Contributor Author

@folletto Are you saying the text should be 14px all-caps?

@alternatekev
Copy link
Contributor Author

@mtias I can go ahead and remove the compactHeader prop that I added here.

@folletto
Copy link
Contributor

Are you saying the text should be 14px all-caps?

Not sure, feels better to me.

Thinking about it, there are a couple of factors at play there:

  1. The size increase is good for readability
  2. It creates a nice coordination having all caps for all the items in the bar (title + buttons)

Which, now that I think of it: if we want the header bigger to make it more readable, doesn't it mean that we should make bigger also the compact buttons? In that way the coordination of the same font will still be possible.

So, if I had to choose... I would probably make bigger both compact buttons and header, both all caps.

But, it's just a preference due to the above I guess. I would be ok with either if you feel strongly about normal case. :)

@mtias
Copy link
Member

mtias commented Feb 1, 2016

So, if I had to choose... I would probably make bigger both compact buttons and header, both all caps.

I wouldn't touch compact-buttons because of this sue case. It's already possible to use regular buttons in section-header as well.

@folletto
Copy link
Contributor

folletto commented Feb 1, 2016

I wouldn't touch compact-buttons because of this sue case. It's already possible to use regular buttons in section-header as well.

I wouldn't either. But it's not a matter of use case: if we are saying that the font is too small, it's too small for both use cases, thus makes sense to change both. ;)

@alternatekev
Copy link
Contributor Author

if we are saying that the font is too small, it's too small for both use cases

I'm not sure I follow the logic that the header text and the button text need to be the same size. The general gist of this PR is that for a header the font is too small. The buttons still seem actionable to me.

@folletto
Copy link
Contributor

folletto commented Feb 1, 2016

I don't get then the need for a bigger size if it's not for readability issues. It works as is for me with a nice coordination between header and buttons. :)

That said, as above, I'm ok with either choice. Just one of the two seems better in my personal opinion, but it's ok either way. :)

@rickybanister
Copy link

The buttons have their own weight due to the border or background of the button so in a way I feel that making the header text larger makes it a better match for the compact buttons. Previously it did match in size, but in a way that was the problem I think @alternatekev is trying to solve. The larger header text stands up better next to the weight of the compact buttons—I think anyway.

@folletto
Copy link
Contributor

folletto commented Feb 1, 2016

👍

@alternatekev alternatekev 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 Feb 1, 2016
@alternatekev alternatekev changed the title SectionHeader: Update default font size and change default behavior to prop SectionHeader: Update default font size Feb 1, 2016
font-size: 14px;
}

.section-header.header-is-compact .section-header__label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we ever apply the header-is-compact class? I did a search for that string using Sublime, and only found this one instance above.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you removed the prop that created this class in the 3rd commit, so let's go ahead and and drop this CSS rule too.

@ebinnion
Copy link
Contributor

ebinnion commented Feb 1, 2016

@alternatekev - I left one comment about some code cruft. Besides that, this LGTM.

@ebinnion ebinnion 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 Feb 1, 2016
@alternatekev
Copy link
Contributor Author

Thanks @ebinnion, I took that out.

@alternatekev alternatekev 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 Feb 2, 2016
@ebinnion ebinnion force-pushed the update/section-header-label-size branch from f31c091 to efecedd Compare February 2, 2016 18:08
@ebinnion
Copy link
Contributor

ebinnion commented Feb 2, 2016

LGTM. 🚢

@ebinnion ebinnion 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 Feb 2, 2016
alternatekev added a commit that referenced this pull request Feb 2, 2016
…size

SectionHeader: Update default font size
@alternatekev alternatekev merged commit daa7bde into master Feb 2, 2016
@ebinnion ebinnion deleted the update/section-header-label-size branch February 2, 2016 19:19
}

.section-header__button {
.section-header__actions button {
Copy link
Member

Choose a reason for hiding this comment

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

This should be .button

@ebinnion
Copy link
Contributor

ebinnion commented Feb 2, 2016

Thanks for pointing that out @mtias. I created #3009 to fix that.

@mtias
Copy link
Member

mtias commented Feb 3, 2016

No problem! Thank you.

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.

None yet

6 participants