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

Remove Gutenberg form control resets and include the styles in the components #22596

Merged
merged 1 commit into from
May 29, 2020

Conversation

youknowriad
Copy link
Contributor

During the weekend, I played a little bit with Gutenberg packages outside WordPress and of the struggles I had is that most form control components relies on some kind of assumed styles that come from WordPress Core (or Gutenberg reset mixin).

This PR tries to embed these styles in the components them selves making them more reusable. It also removes the global reset for inputs, checkboxes... so it's one of those PRs where we'll be on a better position code quality wise but we need to be very careful with testing before merging.

@youknowriad youknowriad added [Feature] UI Components Impacts or related to the UI component system [Type] Code Quality Issues or PRs that relate to code quality labels May 25, 2020
@github-actions
Copy link

Size Change: -19.7 kB (1%)

Total Size: 1.1 MB

Filename Size Change
build/block-editor/index.js 105 kB -16 B (0%)
build/block-editor/style-rtl.css 11.2 kB +310 B (2%)
build/block-editor/style.css 11.2 kB +313 B (2%)
build/block-library/editor-rtl.css 7.55 kB +388 B (5%) 🔍
build/block-library/editor.css 7.56 kB +388 B (5%) 🔍
build/block-library/index.js 119 kB +2 B (0%)
build/components/style-rtl.css 19.5 kB +2.39 kB (12%) ⚠️
build/components/style.css 19.5 kB +2.4 kB (12%) ⚠️
build/edit-post/style-rtl.css 5.43 kB -6.79 kB (124%) 🏆
build/edit-post/style.css 5.43 kB -6.79 kB (124%) 🏆
build/edit-site/style-rtl.css 2.96 kB -2.57 kB (86%) 🏆
build/edit-site/style.css 2.96 kB -2.57 kB (86%) 🏆
build/edit-widgets/style-rtl.css 1.79 kB -2.78 kB (154%) 🏆
build/edit-widgets/style.css 1.79 kB -2.78 kB (154%) 🏆
build/editor/style-rtl.css 4.26 kB -793 B (18%) 👏
build/editor/style.css 4.27 kB -793 B (18%) 👏
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 190 kB 0 B
build/compose/index.js 9.24 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.63 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-site/index.js 13.8 kB 0 B
build/edit-widgets/index.js 7.87 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad
Copy link
Contributor Author

@ItsJonQ @jasmussen any thoughts here?

@ItsJonQ
Copy link

ItsJonQ commented May 28, 2020

I have been rekt by this so many times!

	.input-control, // Upstream name is `.regular-text`.	
	input[type="text"],	
	input[type="search"],	
	input[type="radio"],	
	input[type="tel"],	
	input[type="time"],	
	input[type="url"],	
	input[type="week"],	
	input[type="password"],	
	input[type="checkbox"],	
	input[type="color"],	
	input[type="date"],	
	input[type="datetime"],	
	input[type="datetime-local"],	
	input[type="email"],	
	input[type="month"],	
	input[type="number"],	
	select,

This is a wonderful update @youknowriad

Any suggestions for a good way to test for regressions? (Perhaps specific things to look for?)

Thanks!

@jasmussen
Copy link
Contributor

From what I can tell, and in my careful testing, I can see only upsides and no downsides to this. I'm testing in an opinionated theme with a crazy editor style and things work well. But I do want to echo Q's thought: is there anything in particular to look for?

@youknowriad
Copy link
Contributor Author

Basically, the idea is to check how inputs, checkboxes, radio buttons look across the UI.

@jasmussen
Copy link
Contributor

Yep, so far that's what I tested and it worked well for me. I'm happy to approve, depending on how much testing you feel this needs.

@youknowriad
Copy link
Contributor Author

Please do approve, I'll continue my own testing too and merge.

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 important work, not just for code quality but for componentization. Thanks Riad. Let's get this in and catch any regressions before the next release.

@youknowriad
Copy link
Contributor Author

Let's try this.

@youknowriad youknowriad merged commit 9f45d0b into master May 29, 2020
@youknowriad youknowriad deleted the update/remove-input-resets branch May 29, 2020 10:17
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone May 29, 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 [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants