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-3309 #570

Merged
merged 11 commits into from
Sep 23, 2024
Merged

audit/KAD-3309 #570

merged 11 commits into from
Sep 23, 2024

Conversation

gilbert-hernandez
Copy link
Contributor

@gilbert-hernandez gilbert-hernandez commented Sep 11, 2024

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

Issue Info

  • Were you able to reproduce the issue?
  • Is the issue from the ticket solved? (If not, why?)

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.

Are there dependent changes in another repository?

  • No.
  • Yes. Please provide the link to the PR.

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.

A couple comments, but it's looking good!

@@ -130,9 +142,15 @@ export default function ResponsiveRangeControls( {
<Button
className="is-reset is-single"
isSmall
disabled={ ( ( isEqual( '', value ) ) ? true : false ) }
disabled={ ( ( isEqual( '', value ) ) && ( isEqual( '', tabletValue ) ) && ( isEqual( '', mobileValue ) ) ? true : false ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to compare the currently set values to the default values

For example isEqual( tabletValue, defaultTablet )

{ reset && (
<Button
className="is-reset is-single"
isSmall
disabled={ ( ( isEqual( '', value ) ) ? true : false ) }
disabled={ ( ( isEqual( '', value ) ) && ( isEqual( '', tabletValue ) ) && ( isEqual( '', mobileValue ) ) ? true : false ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to compare the currently set values to the default values

Comment on lines 739 to 742
defaultValue={100}
defaultTablet={100}
defaultMobile={100}
reset={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

In some blocks we import the block.json to access the attributes ( Ex: import metadata from './block.json';)

We should do that here and we can pass the actual default values. Like metadata.attriubtes.topSepHeightTab. If we ever change the default values we don't have to update all the references.

@gilbert-hernandez
Copy link
Contributor Author

I updated the disabled logic for the components, I imported and used metadata for the divider height and widths, and I updated the block.json attributes for the divider width/height default values.

@gilbert-hernandez
Copy link
Contributor Author

I removed the default values I added to the block.json file. I tested it and they are not necessary. I also corrected the default value used for the divider height/width components.

@oakesjosh oakesjosh merged commit 51fde23 into master Sep 23, 2024
15 checks passed
@oakesjosh oakesjosh deleted the audit/KAD-3309 branch September 23, 2024 15:27
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.

2 participants