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 the float on the Spinner to none #19338

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Update the float on the Spinner to none #19338

merged 1 commit into from
Jan 7, 2020

Conversation

mapk
Copy link
Contributor

@mapk mapk commented Dec 27, 2019

Description

Fixes #18199. It appears that changing the float on the spinner component to none instead of right seems to work fine.

How has this been tested?

I tested the visual change in a few places like:

  • Uploading an image to a block.
  • Switching to the Media Library from an Image block.
  • Publishing a post.

Each instance there seemed to work just fine visually. I only tested in Firefox 71.0.

Screenshots

Image block

Screen Shot 2019-12-26 at 5 15 35 PM

Storybook

spinner 2019-12-26 17_19_47

Types of changes

Non-breaking CSS changes.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@mapk mapk self-assigned this Dec 27, 2019
@mapk mapk added Needs Testing Needs further testing to be confirmed. [Feature] UI Components Impacts or related to the UI component system Storybook Storybook and its stories for components labels Dec 27, 2019
@chrisvanpatten
Copy link
Contributor

I would love this to get merged (I’m sick of overriding this in my own code) but there are a few places where the spinner needs to be floated, such as when it’s shown above the compatibility mode metaboxes during saving, which would need to be updated. There are also a bunch of cases where the float is removed via CSS in specific instances, so that CSS would end up being redundant.

@mapk mapk requested a review from ellatrix as a code owner December 30, 2019 17:19
@mapk
Copy link
Contributor Author

mapk commented Dec 30, 2019

@chrisvanpatten, Thanks for the feedback! I checked the meta boxes and that seems to work still.

Screen Shot 2019-12-30 at 9 09 41 AM

I've also updated a couple other files that were overriding the float and removed that style. This still should be tested by others before merging though.

@mapk
Copy link
Contributor Author

mapk commented Jan 4, 2020

Also tested in the link interface and it appears correctly there too.

Screen Shot 2020-01-03 at 5 11 53 PM

@@ -4,7 +4,7 @@
width: 18px;
height: 18px;
opacity: 0.7;
float: right;
float: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I keep this as float:none; or just remove it altogether? @jasmussen any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, if the float does not appear to regress anything, definitely remove it altogether. Floats are the worst, and if there's truly no downside to removing it, out it goes.

A float left or right literally takes an item out of the flow of content, giving it a zero height footprint (with no margins), that would be the only thing to look for, the non floated thing pushing things down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jasmussen ✋ I've removed the float completely now.

@mapk
Copy link
Contributor Author

mapk commented Jan 6, 2020

After removing the float completely, this can use a bit more testing. I've tested on the link creation popover, the uploading of images and it all looks good still.

@jasmussen jasmussen self-requested a review January 7, 2020 08:28
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.

This is working well for me, code looks good and a float really should fix more things than it breaks.

I tested the most obvious spinner locations. From a brief code search it looks like the spinner is used in these places:

  • Block directory search results
  • Media upload progress
  • URL input
  • Block placeholder
  • Embeds loading
  • Galleries loading
  • Images loading
  • Latest posts loading
  • Metaboxes saving
  • Featured images doing something
  • Post publish panel saving

Those are the things to look for.

Removed instances of float:none; in other CSS rules since the component is already set for this.

Remove the float completely.
@mapk mapk force-pushed the try/no-float-spinner branch from 00b49af to 12f693a Compare January 7, 2020 20:15
@mapk mapk merged commit 6312ca7 into master Jan 7, 2020
@mapk mapk deleted the try/no-float-spinner branch January 7, 2020 22:33
@chrisvanpatten
Copy link
Contributor

Thanks @mapk! Excited to put this to use :)

@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Needs Testing Needs further testing to be confirmed. Storybook Storybook and its stories for components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Components: Spinner is oddly floated to the right outside of WordPress
4 participants