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

Polish Social Links block #17380

Merged
merged 3 commits into from
Sep 10, 2019
Merged

Polish Social Links block #17380

merged 3 commits into from
Sep 10, 2019

Conversation

jasmussen
Copy link
Contributor

This PR aims to address recent feedback, and fixes #17362. In doing so it also addresses some initial feedback from #17267, namey to provide a generic "block selected" style.

Specifically this PR:

  • Removes the gray placeholder color, which was intended to provide a "disabled state" for social links that had not been configured.
  • It addresses hover issues reported
  • While I'm here, it refines the code a little bit, adds comments here and there
  • It hides the sibling inserter. Same argument as removing the mover controls, it will be explored separately in context of general child block improvements.
  • It makes the focus style for the selected logo button thicker and darker.
  • The opacity changes that indicate unconfigured blocks are simplified to always provide full opacity on hover or select.
  • It restores and tweaks the "child block selected" outlines, but in a version that is still simpler than the full thing. This will likely benefit from further polish as we seek to make these child block improvements generalized in Nested Blocks: Provide container block prop to absorb child block UI #17267.
  • It hides the "dashed outlines for child blocks" as that clashes with their neutralized margins.

Before:

before

After:

after

@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Sep 9, 2019
@jasmussen jasmussen requested a review from mapk September 9, 2019 08:48
@jasmussen jasmussen self-assigned this Sep 9, 2019
@mapk
Copy link
Contributor

mapk commented Sep 9, 2019

Thanks for working on this, @jasmussen!

  • The hover and focus effect on the parent block feel much better overall. 👍
  • The focused state of the child block is definitely much more clearer. It still feels a tad bit awkward but works. The thick left border helps tremendously here.

One other note, which can be addressed in another PR is the focused state of the block inserter + icon. Should the outline shape be a circle as in the other block inserters?

In Social block

Screen Shot 2019-09-09 at 2 55 23 PM

In other areas of the UI

Screen Shot 2019-09-09 at 2 55 15 PM

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

:shipit:

@jasmussen
Copy link
Contributor Author

Sweet, swift review. I'll merge now to address the big points you raised, and I'll follow up on the focus rectangle for the block library button in a subsequent pr.

@jasmussen jasmussen merged commit 665a102 into master Sep 10, 2019
@jasmussen jasmussen deleted the polish/social-links branch September 10, 2019 06:10
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social block: Odd first experience with the block
3 participants