fix: adjust design in StudioDisplayTile to align with StudioToggleableTextfield design
#14401
Conversation
standeren
added
skip-releasenotes
github-actions
bot
added
area/ui-editor
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14401 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1880 1880
Lines 24446 24447 +1
Branches 2810 2810
=======================================
+ Hits 23386 23387 +1
Misses 800 800
Partials 260 260 ☔ View full report in Codecov by Sentry. |
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css
Outdated
Show resolved
Hide resolved
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.tsx
Outdated
Show resolved
Hide resolved
...mponents/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.module.css
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThe pull request introduces layout and styling changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Nitpick comments (1)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.tsx (1)
31-38: Consider adding a11y improvements.While the structure looks good, consider adding
aria-labelto improve accessibility for screen readers, especially for the icon and value sections.- {icon} + {icon && <span aria-hidden="true">{icon}</span>} <div className={classes.ellipsis}> <Paragraph size='small' className={classes.label}> {label} </Paragraph> - <Paragraph size='small' className={classes.ellipsis}> + <Paragraph size='small' className={classes.ellipsis} aria-label={`${label} value`}> {value} </Paragraph> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css(1 hunks)frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.module.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (6)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.tsx (1)
26-27: LGTM: Clean implementation of prefix icon handling.The addition of
hasPrefixIconand its usage in className composition is well-structured and maintainable.frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.module.css (1)
8-8: Verify text overflow handling with the new padding.The padding change looks good, but based on previous feedback, we should verify that text overflow is handled correctly in edge cases, especially with longer content.
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css (4)
2-3: LGTM: Clean transition to grid layout.The switch from flexbox to grid with
1fr autocolumns provides better control over the layout structure.
9-11: LGTM: Well-structured prefix icon handling.The
.prefixIconclass elegantly handles the additional column for the icon.
21-25: LGTM: Comprehensive text overflow handling.The text overflow properties are well-implemented and consistently applied to both label and ellipsis classes.
13-14: Consider browser compatibility for :has selector.The
:hasselector might not be supported in all browsers. Consider adding a fallback for better browser compatibility.Run the following to check browser support in your target environments:
✅ Verification successful
:hasselector usage aligns with project's browser requirementsThe project's browserslist configuration explicitly targets modern browsers (>0.2% market share, not dead). The
:hasselector is well-supported in all major modern browsers since 2022 and is consistently used across multiple components in the codebase. No fallback is necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check browser support for :has selector curl -s "https://raw.githubusercontent.com/mdn/browser-compat-data/main/css/selectors/has.json" | jq '.browsers'Length of output: 115
Script:
#!/bin/bash # Check for browserslist config in package.json files fd package.json --exec cat {} \; | jq 'select(.browserslist != null) | .browserslist' # Look for other uses of :has selector and potential fallbacks rg ":has\(" --type css -C 2 # Check for any browser support related comments rg -i "browser.?support|fallback|compatibility" --type css -C 2Length of output: 4727
ErlingHauan
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.test.tsx (2)
15-23: Consider adding layout-specific tests.While the basic rendering tests are good, consider adding tests to verify the new vertical alignment layout, as this is a key change mentioned in the PR objectives.
Example test to consider:
it('should render label and value in vertical alignment', () => { render(<StudioDisplayTile {...defaultProps} />); const container = screen.getByLabelText(label).closest('.container'); expect(container).toHaveStyle({ display: 'grid' }); // Add more specific layout assertions based on your CSS implementation });
40-48: Enhance test descriptions for clarity.While the tests correctly verify the prefix icon styling, the descriptions could be more specific about the styling context.
Consider these more descriptive test names:
- it('should not assign prefix icon className by default', () => { + it('should not apply prefix icon spacing when no icon is provided', () => { - it('should assign prefix icon className when prefix icon is set', () => { + it('should apply prefix icon spacing when an icon is provided', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.test.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.tsx
🔇 Additional comments (2)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.test.tsx (2)
7-11: LGTM! Good refactoring of test constants.Extracting
labelandvalueas constants improves maintainability and follows the DRY principle.
25-28: LGTM! Good coverage of padlock icon behavior.The test properly verifies the default visibility of the padlock icon, complementing the existing visibility tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css(1 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.module.css(0 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.module.css
- frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (3)
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css (3)
2-3: LGTM! Base grid layout handles most cases well.The switch to grid layout with
1fr autocolumns effectively handles cases where content needs to be right-aligned (like the lock icon) while allowing the main content to take remaining space.
20-24: LGTM! Text overflow handling follows best practices.The implementation properly handles text truncation with ellipsis, preventing layout breaks from long text while maintaining readability.
12-13: Consider browser compatibility for:has()selector.The
:has()selector is not supported in all browsers. Consider adding a fallback layout for older browsers.Run this script to check current browser support:
✅ Verification successful
:has()selector implementation is correctly handledThe current implementation already provides graceful degradation. The default 2-column grid layout serves as a natural fallback for browsers that don't support the
:has()selector, while modern browsers will enhance to the 3-column layout when needed. No changes required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check browser support for :has() selector curl -s "https://caniuse.com/api/features/css-has" | jq '.data."css-has".support'Length of output: 81
Script:
#!/bin/bash # Get the complete CSS module file content cat frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css # Search for any @supports rules in CSS files related to StudioDisplayTile rg "@supports" frontend/libs/studio-components/src/components/StudioDisplayTile/Length of output: 551
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.module.css
Show resolved
Hide resolved
There was a problem hiding this comment.
Dette er flisespikkeri, men hvis man endrer .displayTile-klassen i ConfigContent.module.css fra
.displayTile {
padding-inline: var(--fds-spacing-5);
}til
.displayTile {
padding-inline: var(--fds-spacing-4);
padding-bottom: var(--fds-spacing-1);
}så synes jeg at det ser litt mer balansert ut (mer på linje med elementene over, pluss at vi får litt mer rom før boksen slutter).
Før:
Etter:
|
|
Ouf, ja, jeg fryktet det 😅 Bra du sjekket! Vanskelig å si om noen bruker den. Det vil jo være synlig for de appene som har >v7 av App backend. Så må sikkert ta hensyn til det og! |
frontend/libs/studio-components/src/components/StudioDisplayTile/StudioDisplayTile.test.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
area/process
|
Så bra ut! Bare å merge for min del 😊 |
Description
Adjust design in
StudioDisplayTileto align withStudioToggleableTextfielddesign.In this PR the label and title for
StudioDisplayTilewill align vertically instead of horizontally, as it is today.The reason for this change is to make it look more like the
StudioToggleableTextfieldwhich we use across the solution.This change is "critical" when working on disabling the codeListId-change in the content library since these fields will be placed in the same view.
Today it looks like this:
In this PR it will look like this:
(It will look the same in SelectDataModelBinding`)
StudioToggleableTextfieldto have a consistent design by removing thechildrenprop and pass thevalueandlabeldirectly.Related Issue(s)
Verification
Summary by CodeRabbit
Style Updates
Layout Improvements
Testing Enhancements
Localization Update
These changes enhance the visual presentation, text handling, and test coverage of the Studio Display Tile component across the application, along with improvements in the Config Viewer Panel layout and localization.