Make WordPress Core

Opened 10 months ago

Closed 6 months ago

#58485 closed defect (bug) (wontfix)

Twenty Twenty-Three: Inline CSS - Unit of measure is redundant

Reported by: ankitmaru's profile ankitmaru Owned by: audrasjb's profile audrasjb
Milestone: Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: css, template, coding-standards Cc:

Description

On a few pages, margin properties have not proper properties as per standards.

CS: Unit of measure is redundant

Change History (20)

This ticket was mentioned in PR #4567 on WordPress/wordpress-develop by @ankitmaru.


10 months ago
#1

Inline CSS fixes on a few templates.

Trac ticket: https://core.trac.wordpress.org/ticket/58485

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

#2 @SergeyBiryukov
10 months ago

  • Component changed from Themes to Bundled Theme
  • Milestone changed from Awaiting Review to 6.3
  • Summary changed from CS: Inline CSS - Unit of measure is redundant to Twenty Twenty-Three: Inline CSS - Unit of measure is redundant

#3 @SergeyBiryukov
10 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 55898:

Twenty Twenty-Three: Remove redundant px unit in a few inline styles.

Follow-up to [54312], [54629].

Props ankitmaru.
Fixes #58485.

@SergeyBiryukov commented on PR #4567:


10 months ago
#4

Thanks for the PR! Merged in r55898.

#5 @TobiasBg
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I haven't tested this, but couldn't this trigger a "block content has changed" warning/error on existing sites? Or wait, rather wouldn't the extra units be coming back as they are still part of the block attributes?

#6 @poena
10 months ago

Hi

This does not cause any validation errors for the block markup in the editor, but I would prefer for it to be reverted:

1) The values in the block comment and the style attribute needs to match. It did not cause a block validation this time, but if it had been a different block attribute like class name or tag name, the mismatch would have caused an error.

2) The block control does not accept unitless values. If we would solve the mismatch and remove the unit from both the comment and the markup, then the block control
in the editor would not display the correct value. I believe this may be why the unit is not removed by the editor when it prepares the CSS.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


9 months ago

#8 @oglekler
9 months ago

@SergeyBiryukov can you address the comment from Carolina?

I browsed the code, and it looks like 0px or 0rem in TwentyTwo theme all over the place, but it is only there. And here is a good point that block attributes and actual block needs to be match. 🤔

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by oglekler. View the logs.


9 months ago

#11 @oglekler
9 months ago

This ticket was discussed during a bug scrub, and comment from @audrasjb: "Given we’re so close to the RC cycle, let’s revert it for now, then move it to milestone 6.4"

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


9 months ago

#13 @audrasjb
9 months ago

  • Owner changed from SergeyBiryukov to audrasjb
  • Status changed from reopened to assigned

As per today's bug scrub: self assigning for revert.

#14 @audrasjb
9 months ago

In 56253:

Twenty Twenty-Three: Revert [55898].

This changeset reverts [55898] which removed redundant px units in a few inline styles, because 1) The values in the block comment and the style
attribute needs to match ; 2) The block control does not accept unitless values.

Follow-up to [54312], [54629], [55898].

Props TobiasBg, poena, oglekler.
See #58485.

#15 @audrasjb
9 months ago

  • Milestone changed from 6.3 to 6.4

Moving for 6.4 consideration.

This ticket was mentioned in Slack in #core by marybaum. View the logs.


7 months ago

#17 @marybaum
7 months ago

From September 5 scrub: we'll test the patch, which I'm sure is perfect, and then maybe next week or sooner we can refer for commit.

#18 @nicolefurlan
7 months ago

@audrasjb do we want to do anything here or should this ticket be closed? It looks like we originally committed the revision to remove the px units, then reverted that change, which is how things stand now.

This ticket was mentioned in Slack in #core by nicolefurlan. View the logs.


6 months ago

#20 @nicolefurlan
6 months ago

  • Milestone 6.4 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

This ticket was discussed in today's bug scrub.

Closing this ticket as wontfix, as the original fix was reverted and it seems like we don't need to pursue any further changes here. Per #comment:6, the px units are needed and intentional by design.

Props to @hellofromTonya :)

Note: See TracTickets for help on using tickets.