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

Migrate TextControl styles from SCSS to CSS-in-JS #18652

Closed
wants to merge 1 commit into from
Closed

Migrate TextControl styles from SCSS to CSS-in-JS #18652

wants to merge 1 commit into from

Conversation

jameslnewell
Copy link
Contributor

@jameslnewell jameslnewell commented Nov 21, 2019

Description

I've migrated the TextControl styles from SCSS to CSS-in-JS.

The reasons I wish to migrate the TextControl styles to CSS-in-JS are:

  • to encapsulate the styles and make the component work anywhere the component is used, even when the base styles aren't used (the base styles are quite opinionated and can make it difficult to use the components outside of gutenberg)
  • to demonstrate theming in a follow-up PR

I've introduced a number of mixins that can be re-used throughout the package as more components begin to use CSS-in-JS e.g. mixins for consistent spacing values. The mixins aren't exposed by the package, but it would likely be useful to expose them once CSS-in-JS is a more accepted direction.

This PR shouldn't introduce any breaking changes unless there is someone using the SCSS seperate from the React component.

There are a few minor visual changes bringing consistency with the proposed typography and spacing systems e.g. padding changed from 6px to 4px and using the standard typography mixins (which aren't responsive like input was).

How has this been tested?

I commented out the style imports (here, here and here) for the storybook (because the base styles have higher specificity) and created a story to confirm the TextControl continues to look the same (minus intentional changes).

Screenshots

Base Control (Before):
image

Base Control (After):

Slightly smaller Help text after using the caption typography style

image

Text Control (Before):
image

Text Control (After):

Slightly smaller Help text after using the caption typography style
*Base styles still apply making the input text smaller than the intended size
image

Text Control (After & with no base-styles):

Slightly smaller Help text after using the caption typography style
Padding changed from 6px to 4px for consistency/adherence with the proposed standard
Larger font size due to typography breakpoint in base-styles - I think this is better discussed/followed-up in a future PR
image

Types of changes

Fixing issues in using the components without the reset - refactoring existing code with no expected breaking changes and no additional API surface area.

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. .

@@ -0,0 +1,46 @@

Copy link
Contributor Author

@jameslnewell jameslnewell Nov 21, 2019

Choose a reason for hiding this comment

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


function BaseControl( { id, label, hideLabelFromVision, help, className, children } ) {
return (
<div className={ classnames( 'components-base-control', className ) }>
<div className="components-base-control__field">
<Control className={ classnames( 'components-base-control', className ) }>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the "unused" classnames for backwards compatibility (they're also used by components that compose this component).

.components-base-control__field {
margin-bottom: $grid-size;

.components-panel__row & {
Copy link
Contributor Author

@jameslnewell jameslnewell Nov 21, 2019

Choose a reason for hiding this comment

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

Moved this style to the Panel component.

@@ -1,4 +0,0 @@
.components-text-control__input {
width: 100%;
padding: 6px 8px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

6px is changed to 4px for consistency.

@ItsJonQ
Copy link

ItsJonQ commented Nov 21, 2019

@jameslnewell This is awesome! Thank you for taking the initiative to do this :).

Since we're starting to add/refactor things with Emotion, I think we have to determine a way of naming/placing style files, as well as the names of the styled-components themselves.

For Card, I had the styles under /styles/card-styles.js. I went with the hyphen based on a suggestion from either @youknowriad or @gziolo (sorry, I can't find the GH comment). As for the styled-components, I added a UI suffix for clarity.

That being said, I'm open to any convention :). As long as we're consistent.

Would love your thoughts

@jameslnewell
Copy link
Contributor Author

jameslnewell commented Nov 21, 2019

@ItsJonQ I've created a seperate issue to continue the discussion around file naming convention and structure (based on previous conversations it is likely a lengthy conversation and be a distraction from the purpose of this PR). When a decision is made, I'll come back and refactor any CSS-in-JS components that have made it into the repository before that decision is made.

use spacing mixins for remaining spacing rules

move panel styling to the panel

use text() mixin

fix use of rgba

autofix linting

fix snapshots

moved files around and reused existing reduceMotion function

changed style file structure and naming
@jameslnewell jameslnewell requested a review from ItsJonQ February 23, 2020 21:43
@ItsJonQ
Copy link

ItsJonQ commented Nov 19, 2020

Closing this one.
Hopefully G2's TextInput will help with this

@ItsJonQ ItsJonQ closed this Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants