chore: display error message to match grid label#4047
chore: display error message to match grid label#4047JamalAlabdullah wants to merge 3 commits intomainfrom
Conversation
JamalAlabdullah
added
kind/other
📝 WalkthroughWalkthroughAdds internal grid-sizing helpers and logic to ComponentStructureWrapper to compute a constrained contentGridSize (respecting a minimum input column width and label/input side-by-side fit) and uses it for component content sizing; a frontend test's flex assertion was made more tolerant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Migrating from UI to YAML configuration.Use the |
JamalAlabdullah
added
kind/feature-request
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/layout/ComponentStructureWrapper.tsx (1)
34-53: Consider making validation Flex sizing consistent with content Flex breakpoints.The validation
Flex(lines 46-51) only specifiesxsandmdbreakpoints, while the contentFlex(line 39) spreadsgrid?.innerGridwhich may includesm,lg, orxlvalues. IfinnerGriddefines narrower widths at these breakpoints, the validation messages could end up wider than the content they relate to, potentially causing visual misalignment.If the intent is for validation to always span full width regardless of
innerGrid, consider being explicit about all breakpoints:<Flex item - size={{ xs: 12, md: 12 }} + size={{ xs: 12, sm: 12, md: 12, lg: 12, xl: 12 }} >Otherwise, if validation width should match content width, consider:
<Flex item - size={{ xs: 12, md: 12 }} + size={{ xs: 12, ...grid?.innerGrid }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/layout/ComponentStructureWrapper.tsx` around lines 34 - 53, componentWithValidations renders two Flex blocks where the content Flex uses size={{ xs: 12, ...grid?.innerGrid }} but the validation Flex hardcodes size={{ xs: 12, md: 12 }}, causing breakpoint mismatch; update the validation Flex sizing to match the content Flex by either spreading grid?.innerGrid (size={{ xs: 12, ...grid?.innerGrid }}) or by explicitly providing the same breakpoints you expect, so AllComponentValidations aligns with the Flex containing children when showValidationMessages is true (refer to componentWithValidations, Flex, AllComponentValidations, grid?.innerGrid, showValidationMessages, indexedId, baseComponentId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/layout/ComponentStructureWrapper.tsx`:
- Around line 34-53: componentWithValidations renders two Flex blocks where the
content Flex uses size={{ xs: 12, ...grid?.innerGrid }} but the validation Flex
hardcodes size={{ xs: 12, md: 12 }}, causing breakpoint mismatch; update the
validation Flex sizing to match the content Flex by either spreading
grid?.innerGrid (size={{ xs: 12, ...grid?.innerGrid }}) or by explicitly
providing the same breakpoints you expect, so AllComponentValidations aligns
with the Flex containing children when showValidationMessages is true (refer to
componentWithValidations, Flex, AllComponentValidations, grid?.innerGrid,
showValidationMessages, indexedId, baseComponentId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6993d1f-db03-494f-9126-c972544c2a20
📒 Files selected for processing (1)
src/layout/ComponentStructureWrapper.tsx
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
One comment about the width👍
I am unsure, but @Magnusrm mentioned that this change can perhaps be a breaking change for existing apps' layouts.
Maybe we should discuss this with the team on slack and see if someone has any more thoughts around this before we merget it.
| {showValidationMessages && ( | ||
| <Flex | ||
| item | ||
| size={{ xs: 12, md: 12 }} |
There was a problem hiding this comment.
| size={{ xs: 12, md: 12 }} | |
| size={{ xs: 12, ...grid?.label?.innerGrid }} |
I may be wrong, but shouldn't the validation message have the same width as label? Edit: you could test if your solution works with setting inner-grid on label, and if it doesnt change the width of error message, you can test my suggestion.
There was a problem hiding this comment.
I have tested in repo ra0479-02 page 5 and it looks no effect happens and everything looks good , i tested in other pages also.
In the layout json files we have some json that has labelGrid and other do not have , so in line 34
const componentWithValidations = !grid?.labelGrid ? (. added logic to cover both status ,
and if you check the component Label.tsx line 57 it is set to 12 ; size={grid ?? { xs: 12 }} this match line 48 in this component ComponentStructureWrapper.tsx .
so the label Flex item uses size={{ xs: 12 }} and size={{ xs: 12, md: 12 }} uses the same, so i think your suggestion is allready included in the ternary logic.
|
|
Hei, |
I added description 👍 |
Superb! The PR is well described, and you have proposed a good solution. My main concern is that this will introduce breaking layout changes for many apps. When I searched for apps using This would also go against the 12-column grid principle our layout system is based on. If an app developer sets I should have responded in the Slack thread when you asked about this earlier, but I forgot to read the responses you got. You could bring this up with the team to see if this is a direction we want to take, supported by the findings @Annikenkbrathen mentioned. If so, we should inform app developers as well and update the documentation. That said, I suspect this kind of breaking change might be received negatively from the app developers, because they must do many changes and we constrain something we earlier offered. An alternative approach could be to let the error message width follow whichever is wider of the input field or the label. This I think solves SSB's issue, but again go against the findings from Anniken. 🤔 |
What do you thing @olemartinorg ? |
|
This sounds risky to me - I agree it's problematic to introduce a breaking change like this new minimum-width. App developers pushed back on changing the PDF font size because it broke their carefully molded layout, and this will definitely break more than that change ever did. Luckily we're already working on the next major version, so breaking changes are possible to introduce, you just have to open a PR in the monorepo instead. That's what will become v10 and the next major version when we release it. As for what is the correct solution, I believe you've looked into it more than I have. I would try to set up a page in one of our test apps with all the possible combinations of grid settings on an input field, and see how (long) validation messages work there (before and after this change). |
3 participants
Description
We have adjusted
ComponentStructureWrapperto follow a design decision which was discussed with design team hereconst MIN_INPUT_COLUMNS = 7;This can be easy changes in future and I made it 7 because 7 cover most validation error message in one line.labelGrid + innerGrid ≤ 12the label and field show side by side; but the error message still follows the field.This achieved by:
-- Calculating whether
labelandinputare side‑by‑side (usesSideBySideLabelAndInput).-- Checking whether
innerGridis too narrow at any breakpoint (isInputTooNarrow).-- Adjusting
innerGridwithgetInnerGridWithMinInputColumnswhen needed, so the field gets at least 7 columns.-- We always render the field and its error messages inside a single Flex (
componentWithValidations) usingcontentGridSizefor its width, and pass that as the only child to Label.”Related Issue(s)
BEFORE
AFTER
Screen.Recording.2026-03-16.at.09.31.28.mov
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Refactor
Tests