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

Fix issue with tertiary button hit areas #10888

Merged
merged 3 commits into from
Oct 25, 2018
Merged

Conversation

jasmussen
Copy link
Contributor

This addressed feedback in #10552 (comment).

It makes the tertiary button style be born with the same hit area as a button, because it is intended to be used as such, just with a different styling.

screenshot 2018-10-22 at 09 03 02

@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Oct 22, 2018
@jasmussen jasmussen added this to the 4.2 milestone Oct 22, 2018
@jasmussen jasmussen self-assigned this Oct 22, 2018
@jasmussen jasmussen requested a review from a team October 22, 2018 07:21
@talldan talldan requested a review from afercia October 23, 2018 06:07
@talldan
Copy link
Contributor

talldan commented Oct 24, 2018

Whoops, was sure I'd reviewed this, but must have closed the tab before commenting.

Anyway, I noticed a couple of things on mobile breakpoints that might need to be fixed. The save icon button is now misaligned with the 'saving' and 'saved' icons.
screen shot 2018-10-24 at 12 42 22 pm
screen shot 2018-10-24 at 12 42 07 pm
screen shot 2018-10-24 at 12 41 55 pm

Then there's also a rogue preview button that pops up momentarily when a non-draft is in the process of being saved, but I don't think it was caused by this PR:
screen shot 2018-10-24 at 12 48 13 pm
screen shot 2018-10-24 at 12 38 53 pm

@jasmussen
Copy link
Contributor Author

Good catch, taking a look.

This addressed feedback in #10552 (comment).

It makes the tertiary button style be born with the same hit area as a button, because it is intended to be used as such, just with a different styling.
@jasmussen
Copy link
Contributor Author

Okay I believe I fixed it. Desktop:

desktop

Mobile:

mobile

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Way better, nothing is worse than tiny button click targets. 👍

@@ -196,6 +196,11 @@
&.is-tertiary {
color: theme(outlines);

// Matches default button in hit area.
Copy link
Member

Choose a reason for hiding this comment

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

Might be handy to document how you got these values, I'm guessing they're based on the size of these tertiary buttons and the gap between them and the default button's size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's from

.

What would be a good comment to point to that?

Copy link
Member

Choose a reason for hiding this comment

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

Huh, so they're basically the same values (and should stay linked, right?)

Using/adding a variable so they don't go out of sync would be good; otherwise you can just reference the file name/line number directly (that GitHub link is fine, even).

Is the lack of 1px padding-bottom intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look at adding a variable, or addressing the line number, or both.

Yes the lack of bottom padding is intentional, it's there for the other button styles because they have a thicker bottom border to imply bevel with topdown shadow, and the bottom padding does something to something there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I clarified the comment a bit, but decided to not variableize anything for now. Reason being, right now we're in that place where some styles are inherited from upstream WordPress styles, and it's a bit messy. We want to move to a place where things are more componentized, and that will require a refactor which should make this file much simpler.

@jasmussen jasmussen merged commit e682faa into master Oct 25, 2018
@jasmussen jasmussen deleted the fix/switch-draft-hitarea branch October 25, 2018 07:35
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
* Fix issue with tertiary button hit areas

This addressed feedback in WordPress#10552 (comment).

It makes the tertiary button style be born with the same hit area as a button, because it is intended to be used as such, just with a different styling.

* Address feedback.

* Clarify comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants