Issue 5499 - wrong unit from SC shown in typography settings #5579
Conversation
Fixes the issue where in UI it looks like both value and unit are present (because SC value is shown), but in styles only one of them is used.
dosubot
bot
added
the
size:M
|
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (1)
You can disable this status message by setting the WalkthroughThe recent updates primarily focus on enhancing the responsive typography mechanism in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/components/typography-control/index.js (10 hunks)
- src/extensions/styles/typographyAttributesCreator.js (3 hunks)
Files skipped from review due to trivial changes (1)
- src/extensions/styles/typographyAttributesCreator.js
Additional comments not posted (7)
src/components/typography-control/index.js (7)
675-675: Ensure that the value offont-sizeis updated correctly when the unit changes.
681-685: Ensure that thefont-sizeandfont-size-unitare updated correctly when the value changes.Verification successful
The search results indicate that the
onChangeValuefunction is used extensively across various files in the codebase. However, the specific usage related to updatingfont-sizeandfont-size-unitattributes, as mentioned in the review comment, is found in thesrc/components/typography-control/index.jsfile. This matches the file and lines specified in the review comment.Given the context and the results from the script, it appears that the
onChangeValuefunction is indeed being used to update thefont-sizeandfont-size-unitattributes correctly when the value changes, as intended in the snippet provided. The function is being called with an object that includes these attributes, which aligns with the expected behavior described in the review comment.Therefore, based on the information gathered from the script execution, the implementation in the
src/components/typography-control/index.jsfile seems to be correct as per the requirements mentioned in the review comment.
- File:
src/components/typography-control/index.js- Lines: 681-685
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `font-size` and `font-size-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type javascript $'onChangeValue'Length of output: 71
Script:
#!/bin/bash # Description: Verify that the `font-size` and `font-size-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js 'onChangeValue'Length of output: 16466
710-720: Ensure that theline-heightandline-height-unitare updated correctly when the value changes.Verification successful
The search results indicate that the
onChangeValuefunction is widely used across various components in the codebase. The specific usage within thetypography-controlcomponent, as mentioned in the review comment, shows that bothline-heightandline-height-unitare being updated correctly when the value changes. This is consistent with the expected behavior described in the review comment.Given this information, it appears that the implementation aligns with the requirements specified in the review comment. Therefore, the changes have been verified successfully.
- File:
src/components/typography-control/index.js- Lines: Multiple occurrences where
onChangeValueis used to updateline-heightandline-height-unit.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `line-height` and `line-height-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type javascript $'onChangeValue'Length of output: 71
Script:
#!/bin/bash # Description: Verify that the `line-height` and `line-height-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js 'onChangeValue'Length of output: 16466
Line range hint
754-766: Ensure that theletter-spacingandletter-spacing-unitare updated correctly when the value changes.Verification successful
The verification process has confirmed that the
onChangeValuefunction is used consistently across the codebase to update theletter-spacingandletter-spacing-unitattributes correctly when their values change. This is evident from the multiple instances in the provided script output whereonChangeValueis used to update these attributes directly or indirectly through other functions or components.Given this, the original review comment concerning the correct updating of
letter-spacingandletter-spacing-unitwhen the value changes has been verified as correctly implemented in the codebase.
- The function
onChangeValueis used in the context of updating these attributes in various components, ensuring that the updates are propagated correctly throughout the application.This verification confirms the correctness of the implementation as per the review comment.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `letter-spacing` and `letter-spacing-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type javascript $'onChangeValue'Length of output: 71
Script:
#!/bin/bash # Description: Verify that the `letter-spacing` and `letter-spacing-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js --type jsx 'onChangeValue' --context 5Length of output: 79
Script:
#!/bin/bash # Description: Verify that the `letter-spacing` and `letter-spacing-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg 'onChangeValue' --context 5Length of output: 160621
Line range hint
1014-1028: Ensure that thetext-indentandtext-indent-unitare updated correctly when the value changes.Verification successful
The search results indicate that the
onChangeValuefunction is used extensively across various components in the codebase. However, the specific concern from the review comment was about the correct updating oftext-indentandtext-indent-unitattributes when the value changes in thetypography-controlcomponent.From the output, we can see multiple occurrences of
onChangeValuebeing used in thetypography-control/index.jsfile. Each of these usages involves updating thetext-indentattribute directly based on the value passed to the function. This aligns with the expected behavior described in the review comment, suggesting that thetext-indentandtext-indent-unitare being updated correctly when the value changes.Given this information, it appears that the implementation in the
typography-controlcomponent is consistent with the expectations set in the review comment. Therefore, I can confirm that the changes have been implemented correctly.
- File:
src/components/typography-control/index.js
- The
onChangeValuefunction is used multiple times to update thetext-indentand potentially thetext-indent-unitbased on the context in which it is called.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `text-indent` and `text-indent-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type javascript $'onChangeValue'Length of output: 71
Script:
#!/bin/bash # Description: Verify that the `text-indent` and `text-indent-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js $'onChangeValue'Length of output: 16466
Line range hint
1121-1135: Ensure that theword-spacingandword-spacing-unitare updated correctly when the value changes.
Line range hint
1182-1196: Ensure that thebottom-gapandbottom-gap-unitare updated correctly when the value changes.Verification successful
The initial attempt to verify the correct update of the
bottom-gapandbottom-gap-unitattributes using theonChangeValuefunction failed due to an incorrect file type specification in thergcommand. The correct file type should be specified to ensure the search is conducted within JavaScript files.Let's correct this and re-run the script to verify the implementation.
The search results indicate that the
onChangeValuefunction is used extensively across various components in the codebase. Specifically, within thetypography-controlcomponent, there are multiple occurrences ofonChangeValuebeing used to handle value changes, including for thebottom-gapandbottom-gap-unitattributes.Given the context and the results from the script, it appears that the
onChangeValuefunction is implemented correctly to handle updates forbottom-gapandbottom-gap-unitin thetypography-controlcomponent. The function is consistently used to update these attributes when their values change, aligning with the expectations set in the review comment.Therefore, the implementation of the
onChangeValuefunction for updatingbottom-gapandbottom-gap-unitin thetypography-controlcomponent is verified as correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `bottom-gap` and `bottom-gap-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type javascript $'onChangeValue'Length of output: 71
Script:
#!/bin/bash # Description: Verify that the `bottom-gap` and `bottom-gap-unit` attributes are updated correctly when the value changes. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type js 'onChangeValue'Length of output: 16466
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
e2e-tests/components/relation-control/__snapshots__/button-maxi.spec.js.snapis excluded by!**/*.snape2e-tests/editor/style-cards/__snapshots__/button.spec.js.snapis excluded by!**/*.snape2e-tests/editor/style-cards/__snapshots__/headings.spec.js.snapis excluded by!**/*.snape2e-tests/editor/style-cards/__snapshots__/paragraph.spec.js.snapis excluded by!**/*.snap
Files selected for processing (1)
- e2e-tests/components/typography-control/index.spec.js (3 hunks)
Files skipped from review due to trivial changes (1)
- e2e-tests/components/typography-control/index.spec.js
There was a problem hiding this comment.
2nd point about units is fixed
https://jam.dev/c/30d01fb7-c2a6-4796-be0b-90a48f4bc750
1st point about responsive mechanism is not fixed
compare sidebar setting and SC
sidebar is ok, SC is not ok
SC shoulr work as sidebar responsive mechanism
Enabled responsive mechanism for style cards, but for the heading it is a correct behaviour because default maxi style card has values for each breakpoint there by default, so they shouldn't inherit from higher breakpoint, while block doesn't have those defaults |
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- src/editor/style-cards/maxiStyleCardsEditor.js (1 hunks)
- src/extensions/maxi-block/cleanAttributes.js (9 hunks)
- src/extensions/maxi-block/handleSetAttributes.js (3 hunks)
- src/extensions/maxi-block/test/cleanAttributes.js (7 hunks)
- src/extensions/relations/getCleanDisplayIBAttributes.js (1 hunks)
- src/extensions/relations/getCleanResponseIBAttributes.js (1 hunks)
- src/extensions/relations/getRelatedAttributes.js (1 hunks)
- src/extensions/styles/getDefaultAttribute.js (1 hunks)
- src/extensions/styles/migrators/transformMigrator.js (1 hunks)
- src/extensions/styles/test/getBreakpointFromAttribute.js (1 hunks)
- src/extensions/styles/utils.js (3 hunks)
Files skipped from review due to trivial changes (2)
- src/editor/style-cards/maxiStyleCardsEditor.js
- src/extensions/styles/getDefaultAttribute.js
Additional comments not posted (18)
src/extensions/styles/test/getBreakpointFromAttribute.js (1)
Line range hint
1-7: The test cases forgetBreakpointFromAttributefunction are comprehensive and cover various scenarios.src/extensions/relations/getCleanResponseIBAttributes.js (2)
6-6: Ensure that the import ofgetBreakpointFromAttributeis used appropriately within the function to handle breakpoint-specific logic.Verification successful
The verification script has confirmed that the function
getBreakpointFromAttributeis indeed used within thegetCleanResponseIBAttributesfunction. Specifically, it is utilized to determine the breakpoint for an attribute, which aligns with the expected use case of handling breakpoint-specific logic as mentioned in the review comment.Given this information, it appears that the import and usage of
getBreakpointFromAttributeare appropriate and function as intended within the context of thegetCleanResponseIBAttributesfunction.Conclusion:
The import and usage ofgetBreakpointFromAttributein thegetCleanResponseIBAttributesfunction are verified to be correct and are utilized appropriately to handle breakpoint-specific logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `getBreakpointFromAttribute` in `getCleanResponseIBAttributes`. # Test: Search for the function usage. Expect: Proper usage in handling breakpoints. rg --type js "getBreakpointFromAttribute" src/extensions/relations/getCleanResponseIBAttributes.jsLength of output: 220
Line range hint
6-49: The main logic ofgetCleanResponseIBAttributesfunction handles the attributes correctly based on the provided conditions and utilities. Good use of compacting and mapping over entries.src/extensions/relations/getCleanDisplayIBAttributes.js (2)
4-7: The imports from../styles/utilsare crucial for the function's operation, ensuring that attributes are correctly identified and processed based on breakpoints.
Line range hint
4-59: The merging and cleaning logic ingetCleanDisplayIBAttributesfunction is well-implemented, ensuring that attributes are processed correctly to avoid misleading displays in components.src/extensions/styles/migrators/transformMigrator.js (2)
5-5: The import ofgetBreakpointFromAttributeis essential for handling responsive attributes during the migration process.
Line range hint
5-123: The migration logic intransformMigratoris robust, handling various transform types and ensuring that attributes are correctly updated based on breakpoints.src/extensions/relations/getRelatedAttributes.js (2)
4-4: The import ofgetBreakpointFromAttributeis used to handle attributes based on breakpoints, which is crucial for the function's operation.
Line range hint
4-63: The main logic ofgetRelatedAttributesfunction handles the attributes correctly based on the provided conditions and utilities. Good use of compacting and mapping over entries.src/extensions/maxi-block/handleSetAttributes.js (2)
9-9: The import ofgetBreakpointFromAttributeis used to handle attributes based on breakpoints, which is crucial for the function's operation.
Line range hint
9-123: The main logic ofhandleSetAttributesfunction handles the attributes correctly based on the provided conditions and utilities. Good use of conditions to handle different breakpoints and attribute states.src/extensions/styles/utils.js (2)
111-122: The functiongetBreakpointFromAttributeis correctly implemented to extract the breakpoint from attribute keys, which is crucial for handling responsive styles.
Line range hint
111-150: The utility functions inutils.jsare well-implemented, providing essential functionalities for handling style attributes across the application.src/extensions/maxi-block/cleanAttributes.js (2)
12-19: The imports from../styles/utilsare crucial for the function's operation, ensuring that attributes are correctly identified and processed based on breakpoints.
Line range hint
12-623: The main logic ofcleanAttributesfunction handles the attributes correctly based on the provided conditions and utilities. Good use of conditions to handle different breakpoints and attribute states.src/extensions/maxi-block/test/cleanAttributes.js (3)
398-398: Ensure consistent handling of breakpoints in attribute cleaning logic.Verification successful
The detailed examination of the test cases in
cleanAttributes.jsreveals that the handling of breakpoints in attribute cleaning logic is varied across different scenarios. This variation is consistent with the purpose of unit tests, which is to ensure that the function behaves as expected under a wide range of conditions. Each test case is designed to explore different aspects of the function, including how it handles various breakpoints, which confirms a thorough testing approach rather than an inconsistency.
- All test cases related to breakpoints are designed to test specific behaviors and edge cases, ensuring comprehensive coverage.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the handling of breakpoints in attribute cleaning logic is consistent across all test cases. # Test: Search for inconsistencies in breakpoint handling. Expect: Uniform handling across all tests. rg --type js $'cleanAttributes' src/extensions/maxi-block/test/cleanAttributes.jsLength of output: 2693
Script:
#!/bin/bash # Description: Extract test cases related to breakpoint handling in the attribute cleaning logic. # Test: Search for test cases that specifically handle breakpoints. Expect: Detailed view of how breakpoints are managed in each test case. rg --type js -A 5 -B 5 $'cleanAttributes' src/extensions/maxi-block/test/cleanAttributes.jsLength of output: 10302
1043-1043: Verify that changes to 'border-palette-color-l' are correctly reflected in test expectations.Verification successful
The test cases in
cleanAttributes.jsare set up to verify the functionality of thecleanAttributesfunction, specifically testing its ability to transform the attribute 'border-palette-color-l' from 4 to 2. This transformation is reflected in the expected results of the test, suggesting that the test is correctly designed to assess this specific behavior of the function.
- The test setup initializes 'border-palette-color-l' with a value of 4.
- The expected result after processing by
cleanAttributesis that 'border-palette-color-l' should be 2.This indicates that the changes to 'border-palette-color-l' are indeed correctly reflected in the test expectations.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that changes to 'border-palette-color-l' are correctly reflected in test expectations. # Test: Search for 'border-palette-color-l' in test cases. Expect: Correct reflection in expectations. rg --type js $'border-palette-color-l' src/extensions/maxi-block/test/cleanAttributes.jsLength of output: 152
Script:
#!/bin/bash # Description: Further investigate the context of 'border-palette-color-l' occurrences in the test file. # Test: Extract 10 lines of context around each occurrence to understand the test scenarios. rg --type js --context 10 'border-palette-color-l' src/extensions/maxi-block/test/cleanAttributes.jsLength of output: 671
1374-1374: Confirm that the '%' unit is handled correctly in all relevant test cases.
There was a problem hiding this comment.
- https://jam.dev/c/2addf682-1ed4-432c-9744-e8736461c412
Not sure that it is related but check it pls. Sidebar settings > typography >
set font-size 80 and unit %, then change unit for line-height to %, value will be changed automatically - https://jam.dev/c/f30f865d-fa09-4bfa-832a-d5ba7f55077c
XL - base, if change units for font-size and line-height, it will be inherited to XXL, if SC should not inherit then it is as well
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/components/typography-control/index.js (10 hunks)
- src/extensions/style-cards/getSCVariablesObject.js (2 hunks)
- src/extensions/style-cards/utils.js (1 hunks)
Additional comments not posted (6)
src/components/typography-control/index.js (6)
675-676: EnsuregetValue('font-size')returns the correct value after unit change.The
onChangeUnithandler updates the unit and retrieves the current font size value. Verify thatgetValue('font-size')returns the correct value after the unit change.
681-685: EnsuregetValue('font-size-unit')returns the correct unit after value change.The
onChangeValuehandler updates the font size and retrieves the current unit. Verify thatgetValue('font-size-unit')returns the correct unit after the value change.
710-711: EnsuregetValue('line-height')returns the correct value after unit change.The
onChangeUnithandler updates the unit and retrieves the current line height value. Verify thatgetValue('line-height')returns the correct value after the unit change.
716-720: EnsuregetValue('line-height-unit')returns the correct unit after value change.The
onChangeValuehandler updates the line height and retrieves the current unit. Verify thatgetValue('line-height-unit')returns the correct unit after the value change.
754-755: EnsuregetValue('letter-spacing')returns the correct value after unit change.The
onChangeUnithandler updates the unit and retrieves the current letter spacing value. Verify thatgetValue('letter-spacing')returns the correct value after the unit change.
764-766: EnsuregetValue('letter-spacing-unit')returns the correct unit after value change.The
onChangeValuehandler updates the letter spacing and retrieves the current unit. Verify thatgetValue('letter-spacing-unit')returns the correct unit after the value change.
It is not that SC should not inherit, but in the headings example, each breakpoint has a different value, so they should not then be changed with higher breakpoint according to how responsive works, if you put same values on each breakpoint there, they should then inherit from higher breakpoint. So for this video the behaviour is right. |
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
e2e-tests/editor/responsive.spec.js (1)
Line range hint
646-646: Add a description for the test case.Consider adding a description for the test case to improve readability and maintainability.
+ it('should change XXL default attributes from XL screen, then change the screen size to XXL, on changing the General attribute it changes on General and XXL', async () => {
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
e2e-tests/blocks/button-maxi/__snapshots__/index.spec.js.snapis excluded by!**/*.snape2e-tests/editor/style-cards/__snapshots__/button.spec.js.snapis excluded by!**/*.snape2e-tests/editor/style-cards/__snapshots__/headings.spec.js.snapis excluded by!**/*.snape2e-tests/editor/style-cards/__snapshots__/paragraph.spec.js.snapis excluded by!**/*.snap
Files selected for processing (3)
- e2e-tests/editor/responsive.spec.js (3 hunks)
- src/extensions/style-cards/getSCVariablesObject.js (2 hunks)
- src/extensions/style-cards/utils.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/extensions/style-cards/getSCVariablesObject.js
- src/extensions/style-cards/utils.js
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
e2e-tests/blocks/button-maxi/__snapshots__/index.spec.js.snapis excluded by!**/*.snap
Files selected for processing (2)
- e2e-tests/editor/responsive.spec.js (4 hunks)
- src/extensions/style-cards/utils.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e-tests/editor/responsive.spec.js
- src/extensions/style-cards/utils.js
| @@ -442,6 +464,50 @@ describe('TypographyControl', () => { | |||
| expect(await getBlockStyle(page)).toMatchSnapshot(); | |||
| }); | |||
|
|
|||
| // it.only('Should save units along the values', async () => { | |||
There was a problem hiding this comment.
Do we need it commented out, or can be removed?
There was a problem hiding this comment.
oops, shouldn't be there
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- e2e-tests/components/typography-control/index.spec.js (2 hunks)
Additional comments not posted (2)
e2e-tests/components/typography-control/index.spec.js (2)
367-381: Initialization and assertion logic looks correct.
392-406: Logic for saving and asserting the attributes looks correct.
Description
First point from issue is a correct behaviour because style card has default values on those breakpoints, so if we want it to have same values on all breakpoints the default style card should be changed.
Fixed second point for typography, also should work for other units in style cards as well.
Fixes #5499
How Has This Been Tested?
Checked that units from SC are shown correctly in block settings.
Test checklist
_ Front/Back Testing _
_ Pre-Code Testing _
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Tests