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

Add styles for Spinner component #8801

Merged
merged 7 commits into from Aug 13, 2018

Conversation

Projects
None yet
4 participants
@mmtr
Contributor

mmtr commented Aug 9, 2018

Closes #8566.

It adds the needed styles by the Spinner component, so it is displayed correctly even if the WordPress Core styles are not available.

Note that the spinner provided by the WordPress Core styles uses an image. In order to don't handle any image during the build process, this component renders a spinner using only CSS that should look like the original image.

aug-10-2018 13-44-35
(Left: Original Image - Right: New CSS only)

@mmtr

This comment has been minimized.

Show comment
Hide comment
@mmtr
Contributor

mmtr commented Aug 9, 2018

const spinner = <span className="spinner is-active" />;
export default () => spinner;
export default () => <span className="components-spinner" />;

This comment has been minimized.

@gziolo

gziolo Aug 10, 2018

Member

It's some kind of micro-optimization recommended by Facebook.

@gziolo

gziolo Aug 10, 2018

Member

It's some kind of micro-optimization recommended by Facebook.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 10, 2018

Member

@karmatosed @jasmussen - can you confirm if the proposed approach is expected? There are a few similar components which need some action to be reusable outside of WordPress.

Member

gziolo commented Aug 10, 2018

@karmatosed @jasmussen - can you confirm if the proposed approach is expected? There are a few similar components which need some action to be reusable outside of WordPress.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 10, 2018

Contributor

I think Riad's instinct is right — we are building new components for future JavaScript powered WordPress components, and they need to be able to not have to rely on WordPress core styles. Long term we migth even want to redesign them but for now even if they duplicate the CSS they should look the same as WordPress core unless we have a good reason, and if we have a good reason we should track it on https://core.trac.wordpress.org/ticket/44749

Contributor

jasmussen commented Aug 10, 2018

I think Riad's instinct is right — we are building new components for future JavaScript powered WordPress components, and they need to be able to not have to rely on WordPress core styles. Long term we migth even want to redesign them but for now even if they duplicate the CSS they should look the same as WordPress core unless we have a good reason, and if we have a good reason we should track it on https://core.trac.wordpress.org/ticket/44749

@gziolo gziolo requested a review from youknowriad Aug 10, 2018

@gziolo gziolo added this to the 3.6 milestone Aug 10, 2018

@youknowriad

I like that it's only CSS 👏 I left some small comments but looks good otherwise.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 10, 2018

Contributor

looks like there's a stylelint issue :)

Contributor

youknowriad commented Aug 10, 2018

looks like there's a stylelint issue :)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 10, 2018

Contributor

Sorry for introducing those conflicts, do you mind running a rebase, I'll merge right after? Thanks

Contributor

youknowriad commented Aug 10, 2018

Sorry for introducing those conflicts, do you mind running a rebase, I'll merge right after? Thanks

mmtr added some commits Aug 9, 2018

@mmtr

This comment has been minimized.

Show comment
Hide comment
@mmtr

mmtr Aug 10, 2018

Contributor

Sorry for introducing those conflicts, do you mind running a rebase, I'll merge right after? Thanks

Done!

Contributor

mmtr commented Aug 10, 2018

Sorry for introducing those conflicts, do you mind running a rebase, I'll merge right after? Thanks

Done!

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 10, 2018

Contributor

Travis is very unstable at the moment, it doesn't let me merge this PR :)

Contributor

youknowriad commented Aug 10, 2018

Travis is very unstable at the moment, it doesn't let me merge this PR :)

mmtr added some commits Aug 10, 2018

@youknowriad youknowriad merged commit 6ce1a6b into WordPress:master Aug 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment