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

audit/KAD-4008 #646

Merged
merged 10 commits into from
Jan 20, 2025
Merged

audit/KAD-4008 #646

merged 10 commits into from
Jan 20, 2025

Conversation

gilbert-hernandez
Copy link
Contributor

🎫 #[Jira Ticket]
https://stellarwp.atlassian.net/browse/KAD-4008
...

Checklist

  • I have performed a self-review.
  • No unrelated files are modified.
  • No debugging statements exist (Ex: console.log, error_log).
  • There are no warnings or notices in the wordpress error log.
  • Passes all tests (linting, acceptance, & unit)

Block specific checklist (where relevant)

  • Tested with an existing instance of this block .
  • Tested creating a new instance of this block.
  • Tested with Dynamic content & Elements.

@gilbert-hernandez
Copy link
Contributor Author

This adds the em/rem sizes to the icon border and border radius. It also updates the .5 step to .1 for the stroke width.

Copy link
Contributor

@oakesjosh oakesjosh left a comment

Choose a reason for hiding this comment

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

As you mentioned in Jira, this doesn't work for number and image media styles in the editor. We'll want to update this PR to support these media styles in the editor too.

I appears to be working fine across all media styles on the front end.

@gilbert-hernandez
Copy link
Contributor Author

Image Border now uses em/rem

Image Border Radius now uses em/rem

Number Size now uses em/rem

Number Border uses em/rem

Number Border Radius uses em/rem

Fixed an issue where the numbers didn’t show in frontend.

@gilbert-hernandez
Copy link
Contributor Author

@oakesjosh

I removed the changes to the save.js file and made a new ticket for that particular problem.

Also, I updated the checks for the mediaIcon[0].borderRadiusUnit for the max attribute in some of the settings as we discussed. The ?. operator returns undefined if the value isn't there. In this case, if the value is undefined, the max needs to be for a pixel value. So, if it is undefined (because it didn't exist before the update) the max should be the pixel value instead of the EM/REM value.

In other places, I used the ?? operator, so I think these changes should avoid runtime errors and still provide a truthy response for backward compatibility. If a user has already set a size before the update, they should still be able to update the slide without running into a problem where the max value is 12px.

@oakesjosh oakesjosh merged commit 9222109 into master Jan 20, 2025
20 checks passed
@oakesjosh oakesjosh deleted the audit/KAD-4008 branch January 20, 2025 14:55
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.

3 participants