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

Jump to bottom

Fix: empty style attribute issue in navigation block #62600

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Fix: empty style attribute issue in navigation block #62600

merged 4 commits into from
Jun 18, 2024

Conversation

up1512001
Copy link
Member

What?

  • checking if $colors['overlay_inline_styles'] is not empty then only adding style attribute.

Why?

#62358

How?

( ! empty( esc_attr( safecss_filter_attr( $colors['overlay_inline_styles'] ) ) ) ) ? ( 'style="' . esc_attr( safecss_filter_attr( $colors['overlay_inline_styles'] ) ) . '"' ) : ''

Testing Instructions

  • create page
  • add navigation block
  • see div with wp-block-navigation__responsive-container previously having empty style will not have style attribute.

Screenshots or screencast

Screenshot 2024-06-15 at 23 51 01

@up1512001 up1512001 requested a review from ajitbohra as a code owner June 15, 2024 18:21
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 15, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @convolutedkdlkf.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: convolutedkdlkf.

Co-authored-by: up1512001 <[email protected]>
Co-authored-by: artemiomorales <[email protected]>
Co-authored-by: talldan <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@artemiomorales artemiomorales added the [Type] Bug An existing feature does not function as intended label Jun 16, 2024
artemiomorales
artemiomorales reviewed Jun 16, 2024
View reviewed changes
Copy link
Contributor

@artemiomorales artemiomorales left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I tested this and it works as expected 👍

That being said, I'm not sure it's necessary to remove the empty style tag or if there may be unforeseen side effects, so I'll defer approval until we get other thoughts and/or someone with more context on the navigation block can weigh in. It seems like this shouldn't cause any issues though, and it does produce cleaner markup.

@talldan talldan added the [Block] Navigation Affects the Navigation Block label Jun 17, 2024
@talldan talldan linked an issue Jun 17, 2024 that may be closed by this pull request
The Navigation Block has an Inline Style Without Declarations. #62358
Closed
@up1512001 up1512001 requested a review from talldan June 17, 2024 08:21
talldan
talldan reviewed Jun 18, 2024
View reviewed changes
packages/block-library/src/navigation/index.php Outdated
@@ -537,7 +539,7 @@ private static function get_responsive_container_markup( $attributes, $inner_blo
$toggle_aria_label_close,
esc_attr( implode( ' ', $responsive_container_classes ) ),
esc_attr( implode( ' ', $open_button_classes ) ),
esc_attr( safecss_filter_attr( $colors['overlay_inline_styles'] ) ),
( ! empty( $overlay_inline_styles ) ) ? "style=$overlay_inline_styles" : '',
Copy link
Contributor

@talldan talldan Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The PR isn't working at the moment, this is just missing some quotes around the style to make it a valid html attribute:

Suggested change
( ! empty( $overlay_inline_styles ) ) ? "style=$overlay_inline_styles" : '',
( ! empty( $overlay_inline_styles ) ) ? "style=\"$overlay_inline_styles\"" : '',

To reproduce:

  1. Set a navigation block to Overlay - 'Always'
  2. Add some custom colors for the overlay menu
  3. Save and view the frontend - observe the custom colors aren't applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

@talldan added missing quotes.
Thanks for suggestion.

talldan
talldan approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for your contribution @up1512001!

@talldan talldan merged commit 4907090 into WordPress:trunk Jun 18, 2024
60 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.7 milestone Jun 18, 2024
@talldan talldan added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 18, 2024
ellatrix pushed a commit that referenced this pull request Jun 25, 2024
@up1512001 @artemiomorales
@talldan @ellatrix
Fix: empty style attribute issue in navigation block (#62600)
f6c7337 
* Fix: empty style attribute issue in navigation block

* update: store overlay style to variable for better readibility and performance

* Fix: linting issue

* Fix: added missing quotes to make valid markup

----

Unlinked contributors: convolutedkdlkf.

Co-authored-by: up1512001 <[email protected]>
Co-authored-by: artemiomorales <[email protected]>
Co-authored-by: talldan <[email protected]>
@ellatrix
Copy link
Member

I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: f6c7337

@ellatrix ellatrix added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jun 25, 2024
ellatrix pushed a commit that referenced this pull request Jun 25, 2024
@up1512001 @artemiomorales
@talldan @ellatrix
Fix: empty style attribute issue in navigation block (#62600)
5460ed3 
* Fix: empty style attribute issue in navigation block

* update: store overlay style to variable for better readibility and performance

* Fix: linting issue

* Fix: added missing quotes to make valid markup

----

Unlinked contributors: convolutedkdlkf.

Co-authored-by: up1512001 <[email protected]>
Co-authored-by: artemiomorales <[email protected]>
Co-authored-by: talldan <[email protected]>
@jeffpaul
Copy link
Member

jeffpaul commented Nov 8, 2024

@convolutedkdlkf mind sharing your WordPress.org username so I can ensure it appears in the 6.7 credits listing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artemiomorales artemiomorales artemiomorales left review comments

@talldan talldan talldan approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@up1512001 up1512001

Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 18.7
Development

Successfully merging this pull request may close these issues.

The Navigation Block has an Inline Style Without Declarations.
5 participants
@up1512001 @ellatrix @jeffpaul @talldan @artemiomorales