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.

Sign up for GitHub

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 stylelint rule to prevent usage of flex-direction reverse values #63081

Merged
merged 12 commits into from
Jul 30, 2024
Merged

Add stylelint rule to prevent usage of flex-direction reverse values #63081

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
16c726d
Add Stylelint rule to prevent usage of flex-direction *-reverse values.
afercia Jul 2, 2024
c1ce569
Add Stylelint disable comments for valid cases or cases that should b…
afercia Jul 2, 2024
db35911
Remove row-reverse from experimental checkbox form input.
afercia Jul 3, 2024
af868fa
Add disable comment for the hooked blocks toggles.
afercia Jul 3, 2024
9efa055
Remove leftovers.
afercia Jul 3, 2024
7b6abac
Fix typo.
afercia Jul 22, 2024
00f59d5
Use regex.
afercia Jul 23, 2024
113fc0e
Merge branch 'trunk' into add/stylelint-rule-flex-reverse
t-hamano Jul 26, 2024
c4c55b2
Revert changes to `form-input/*`
t-hamano Jul 26, 2024
d47840d
Revert changes to `form-input/edit.js`
t-hamano Jul 26, 2024
5190d82
Update .stylelintrc.json
t-hamano Jul 26, 2024
81c8982
Merge branch 'trunk' into add/stylelint-rule-flex-reverse
t-hamano Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion .stylelintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,20 @@
"at-rule-empty-line-before": null,
"at-rule-no-unknown": null,
"comment-empty-line-before": null,
"declaration-property-value-allowed-list": [
{
"flex-direction": [
"row",
"column",
"inherit",
"initial",
"unset"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to allow variables as values?

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to do it with regex. I think it might be easier if we just list all the possible reverse values and with a negative regex (untested):

Suggested change
"row",
"column",
"inherit",
"initial",
"unset"
"/-reverse$/",

That will also allow using variables as @youknowriad raised above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyxla sure but in order to do that we should use declaration-property-value-disallowed-list which is already used. It's not clear to me how to add multiple disallowed properties with custom messages under the same Stylelint rule. Any clue?

Copy link
Member

@mirka mirka Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

add multiple disallowed properties with custom messages under the same Stylelint rule

This is where we need to first convert the config file from JSON to JS, as well as update the Stylelint dependency to a newer version that supports custom message functions for this rule (#63090).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to converting it. @mirka would you advise to wait for the conversion to happen or maybe proceed with this PR as is, and then iterate when #63090 will be solved?

Copy link
Member

Choose a reason for hiding this comment

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

@tyxla sure but in order to do that we should use declaration-property-value-disallowed-list which is already used. It's not clear to me how to add multiple disallowed properties with custom messages under the same Stylelint rule. Any clue?

Good point, but I think you should be able to do it with declaration-property-value-allowed-list, just need to invert the regex to match anything BUT row-reverse and column-reverse, for example:

"/^(?!(row|column)-reverse).*$/"

Haven't tested it, so let me know if that works for you.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, converting to the JS format makes sense, although it doesn't look like a high-priority task.

]
},
{
"message": "Avoid the flex-direction reverse values. For accessibility reasons, visual, reading, and DOM order must match. Only use the reverse values when they do not affect reading order, meaning, and interaction,"
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
}
],
"declaration-property-value-disallowed-list": [
{
"/.*/": [ "/--wp-components-color-/" ]
Expand All @@ -18,7 +32,7 @@
"property-disallowed-list": [
[ "order" ],
{
"message": "Avoid the order property. For accessibility reasons, visual, reading, and DOM order must match. Only use the order property when it does not affect reading order, meaning, and interaction"
"message": "Avoid the order property. For accessibility reasons, visual, reading, and DOM order must match. Only use the order property when it does not affect reading order, meaning, and interaction,"
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
}
],
"rule-empty-line-before": null,
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/block-hooks.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* we need to right-align the toggle.
*/
.components-toggle-control .components-h-stack {
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This should be refactored to not use the row-reverse value. */
flex-direction: row-reverse;
}

Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/use-editor-wrapper-styles.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
}

.use-editor-wrapper-styles--reversed {
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This should be refactored to not use the column-reverse value. */
flex-direction: column-reverse;
width: 100%;
max-width: 580;
Expand Down
44 changes: 33 additions & 11 deletions packages/block-library/src/form-input/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ function InputFieldBlock( { attributes, setAttributes, className } ) {
ref.current.focus();
}

// Note: radio inputs aren't implemented yet.
const isCheckboxOrRadio = type === 'checkbox' || type === 'radio';

const controls = (
<>
{ 'hidden' !== type && (
Expand Down Expand Up @@ -107,17 +110,21 @@ function InputFieldBlock( { attributes, setAttributes, className } ) {
'is-label-inline': inlineLabel || 'checkbox' === type,
} ) }
>
<RichText
tagName="span"
className="wp-block-form-input__label-content"
value={ label }
onChange={ ( newLabel ) =>
setAttributes( { label: newLabel } )
}
aria-label={ label ? __( 'Label' ) : __( 'Empty label' ) }
data-empty={ label ? false : true }
placeholder={ __( 'Type the label for this input' ) }
/>
{ ! isCheckboxOrRadio && (
<RichText
tagName="span"
className="wp-block-form-input__label-content"
value={ label }
onChange={ ( newLabel ) =>
setAttributes( { label: newLabel } )
}
aria-label={
label ? __( 'Label' ) : __( 'Empty label' )
}
data-empty={ label ? false : true }
placeholder={ __( 'Type the label for this input' ) }
/>
) }
<TagName
type={ 'textarea' === type ? undefined : type }
className={ clsx(
Expand All @@ -143,6 +150,21 @@ function InputFieldBlock( { attributes, setAttributes, className } ) {
...colorProps.style,
} }
/>
{ isCheckboxOrRadio && (
<RichText
tagName="span"
className="wp-block-form-input__label-content"
value={ label }
onChange={ ( newLabel ) =>
setAttributes( { label: newLabel } )
}
aria-label={
label ? __( 'Label' ) : __( 'Empty label' )
}
data-empty={ label ? false : true }
placeholder={ __( 'Type the label for this input' ) }
/>
) }
</span>
</div>
);
Expand Down
16 changes: 13 additions & 3 deletions packages/block-library/src/form-input/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export default function save( { attributes } ) {

const blockProps = useBlockProps.save();

// Note: radio inputs aren't implemented yet.
const isCheckboxOrRadio = type === 'checkbox' || type === 'radio';

if ( 'hidden' === type ) {
return <input type={ type } name={ name } value={ value } />;
}
Expand All @@ -67,9 +70,11 @@ export default function save( { attributes } ) {
'is-label-inline': inlineLabel,
} ) }
>
<span className="wp-block-form-input__label-content">
<RichText.Content value={ label } />
</span>
{ ! isCheckboxOrRadio && (
<span className="wp-block-form-input__label-content">
<RichText.Content value={ label } />
</span>
) }
<TagName
className={ inputClasses }
type={ 'textarea' === type ? undefined : type }
Expand All @@ -79,6 +84,11 @@ export default function save( { attributes } ) {
placeholder={ placeholder || undefined }
style={ inputStyle }
/>
{ isCheckboxOrRadio && (
<span className="wp-block-form-input__label-content">
<RichText.Content value={ label } />
</span>
) }
</label>
{ /* eslint-enable jsx-a11y/label-has-associated-control */ }
</div>
Expand Down
13 changes: 5 additions & 8 deletions packages/block-library/src/form-input/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,13 @@
}
}

/*
Small tweak to left-align the checkbox.
Even though `:has` is not currently supported in Firefox, this is a small tweak
and does not affect the functionality of the block or the user's experience.
There will be a minor inconsistency between browsers. However, it's more important to provide
a better experience for 80+% of users, until Firefox catches up and supports `:has`.
*/
&:has(input[type="checkbox"]) {
width: fit-content;
flex-direction: row-reverse;

.wp-block-form-input__input,
.wp-block-form-input__label-content {
margin: 0;
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/media-text/style.native.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ $media-to-text: 12px;
}

.has-media-on-the-right {
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This should be refactored to not use the row-reverse value. */
flex-direction: row-reverse;
}

.is-stacked-on-mobile {
flex-direction: column;

&.has-media-on-the-right {
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This should be refactored to not use the column-reverse value. */
flex-direction: column-reverse;
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/navigation/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@ button.wp-block-navigation-item__content {
.wp-block-navigation__container {
display: flex;
flex-wrap: var(--navigation-layout-wrap, wrap);
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This variable value does not use row-reverse or column-reverse. */
flex-direction: var(--navigation-layout-direction, initial);
justify-content: var(--navigation-layout-justify, initial);
align-items: var(--navigation-layout-align, initial);
Expand Down Expand Up @@ -484,6 +485,7 @@ button.wp-block-navigation-item__content {
.wp-block-navigation__responsive-container-content {
display: flex;
flex-wrap: var(--navigation-layout-wrap, wrap);
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This variable value does not use row-reverse or column-reverse. */
flex-direction: var(--navigation-layout-direction, initial);
justify-content: var(--navigation-layout-justify, initial);
align-items: var(--navigation-layout-align, initial);
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/page-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
.wp-block-navigation {
.wp-block-page-list {
display: flex;
/* stylelint-disable-next-line declaration-property-value-allowed-list -- This variable value does not use row-reverse or column-reverse. */
flex-direction: var(--navigation-layout-direction, initial);
justify-content: var(--navigation-layout-justify, initial);
align-items: var(--navigation-layout-align, initial);
Expand Down
Loading