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

Update sidebar spacing #13181

Merged
merged 4 commits into from
Feb 6, 2019

Conversation

geekpulp
Copy link
Contributor

@geekpulp geekpulp commented Jan 3, 2019

Description

Fixes #12652.

Fixing a spacing issue between block controls in the sidebar (issue #12652). I had previously submitted a pull request (PR #13106) for this but managed to screw up my repo (a rookie mistake) so I'm resubmitting now that I have a better idea of what I'm doing :)

How has this been tested?

Testing in firefox, safari, and chrome

Testing in the standard docker environment

Screenshots

Before

Types of changes

Visual change to the sidebar which increases the spacing between elements

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@Soean
Copy link
Member

Soean commented Jan 3, 2019

What have you fixed exactly? Can you add a before/after screenshot?

Edit:
Old PR: #13106
Issue: #12652

@gziolo gziolo added General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement. labels Jan 31, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Jan 31, 2019
@gziolo
Copy link
Member

gziolo commented Jan 31, 2019

@geekpulp - can you add the screenshot presenting changes after your changes applied? It looks like your first code contribution might land into Gutenberg quite soon :)

@gziolo gziolo requested review from mapk and jasmussen January 31, 2019 15:12
@geekpulp
Copy link
Contributor Author

geekpulp commented Feb 1, 2019

@geekpulp - can you add the screenshot presenting changes after your changes applied? It looks like your first code contribution might land into Gutenberg quite soon :)

I'm embarrassingly excited by that. I should get that sorted for you in the next 48 hours.

@jasmussen
Copy link
Contributor

Thanks so much for this PR! Really happy to see this.

It needs a little bit of work, though. I'm really really happy to see so much use of variables like $grid-size. However we might want to pull back on as many of those changes as we can, in order to be able to ship this, because there are a few regressions introduced as a result of this. Here's master:

screenshot 2019-02-01 at 09 56 19

Here's this branch:

screenshot 2019-02-01 at 09 55 34

Notice how the separators no longer go edge to edge, and how the padding is kind of compact. It looks like the following rule is the crux of the fix:

.components-base-control + .components-base-control {
	margin-bottom: $grid-size-large;
}

but either this is not working correctly, or we might want to increase it. Here's master:

screenshot 2019-02-01 at 09 56 36

Here's this branch:

screenshot 2019-02-01 at 09 57 20

I'll try and leave some thoughts in the code as to how we might address this in a simpler way perhaps.

@geekpulp
Copy link
Contributor Author

geekpulp commented Feb 1, 2019

Ok, I've made some changes and it seems to be ok from what I can see. Here's what it's looking like for me:

screen shot 2019-02-02 at 9 04 19 am

Strangely @jasmussen, I couldn't see the regression issues you showed in your screenshots? I tried in safari, chrome, and firefox but couldn't recreate it. This makes me think I'm doing something wrong with my dev environment?

@jasmussen
Copy link
Contributor

Very nice work, thank you. This is 90% there, but this might be one of those cases where the last 10% might take a while. Here's an index of the sidebars:

screenshot 2019-02-04 at 11 31 05

screenshot 2019-02-04 at 11 31 16

screenshot 2019-02-04 at 11 31 25
screenshot 2019-02-04 at 11 31 47
screenshot 2019-02-04 at 11 32 02

screenshot 2019-02-04 at 11 32 08

screenshot 2019-02-04 at 11 32 24

screenshot 2019-02-04 at 11 32 30
screenshot 2019-02-04 at 11 32 42
screenshot 2019-02-04 at 11 32 50

Notice how every collapsed panel in the block settings sidebar has a bottom margin that it shouldn't have.

It also appears like some controls (perhaps the ones that DO use BaseControl as a wrapper, not all elements do) have a relaxed bottom margin, whereas others are a bit tight.

It's a little frustrating that we need to account for so many edgecases, it'd be nice if there was a simple fix across all these. Let me know if you have time to take a stab, otherwise I can take over this PR and bring it home.

@geekpulp
Copy link
Contributor Author

geekpulp commented Feb 5, 2019

@jasmussen I hate to say this but I don't think I will have any time on this until the weekend. Happy for you to progress it from here.

geekpulp and others added 4 commits February 6, 2019 10:47
Co-Authored-By: geekpulp <glen@geekpulp.co.nz>
Increased the size of some spacing to allow more room between elements.
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Okay, I commited a fix, and rebased your branch. As far as I'm concerned, this is now good to go. Let's see if the tests don't pass.

GIF:

sidebar

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

@geekpulp congratulations on your first code contribution to Gutenberg 🎉

@gziolo gziolo merged commit d82a3bc into WordPress:master Feb 6, 2019
@geekpulp
Copy link
Contributor Author

geekpulp commented Feb 6, 2019

That’s everyone for your help on this. Huge learning experience for me. First contribution out of the way, on to the next!

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Update sidebar spacing

* Update packages/components/src/base-control/style.scss

Co-Authored-By: geekpulp <glen@geekpulp.co.nz>

* updated use of $grid-size

Increased the size of some spacing to allow more room between elements.

* Commit fixes
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Update sidebar spacing

* Update packages/components/src/base-control/style.scss

Co-Authored-By: geekpulp <glen@geekpulp.co.nz>

* updated use of $grid-size

Increased the size of some spacing to allow more room between elements.

* Commit fixes
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad sidebar style (no spacing)
4 participants