chore: display error message to match grid label#4047
chore: display error message to match grid label#4047JamalAlabdullah wants to merge 11 commits intomainfrom
Conversation
JamalAlabdullah
added
kind/other
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDerives breakpoint-aware grid sizing in ComponentStructureWrapper, adds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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.
|
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). |
|
@lassopicasso The suggestion from Ole Martin is ready to review and test |
There was a problem hiding this comment.
It doesn't seem to work when I set the config. The validation still follows the same width as the input field. Am I doing something wrong? I suspect something unexpected might be happening around inputGridSizeAndExactWidth.
I tested in frontend-test app in the first input field in form layout.
Config:
Result:
I’m also finding it a bit difficult to follow the flow here, so could you explain the logic behind the decisions in ComponentStructureWrapper in details, especially inputGridSizeAndExactWidth? I would try to avoid the number of abstraction levels. And a nitpick; from the naming, I would expect this to apply only to the input, but it also set the validation width.
I'm also curious why we need to set specific grid breakpoints and styling here, when Flex already handles this based on the size prop.
My initial thought is that the validation width logic could potentially be handled at a lower level when showValidationMessages is defined, but I may be missing some context.
What I think is happening now:
Flexwrapper sets the max grid size usingcontentGridSizecontentGridSizeis set to the largest of innerGrid and validationGrid- The inner Flex components then cannot exceed the outer Flex grid size
If you need a quick review before we go on vacation on friday (I'll be away for 3 weeks), it might be helpful to also check with someone in same time zone for another round. I’ll likely only have time for one more review, and because of the time difference I unfortunately won’t be able to provide quick follow-up.
I did not know why it did not worked when you tested because i tested in same app and it worked, any way i refactored and removed the entire |
|
@olemartinorg har du mulighet bare for å teste 😄 ? |
There was a problem hiding this comment.
Great job! The code looks much better now and it's easier to follow the flow.
I have a few comments, but overall the changes look good 👍
I also tested it, and it seems I can now set a custom width for the validation message. However, it looks like the input and validation together only take half of the width they’re supposed to.
For example, this config should take half of the layout width, but the input and validation message only take half of that again. So it seems like the width is being applied twice somewhere. I compared it with main branch to make sure my app isn't wrong, and in main it took 50% width of the layout.
"grid": {
"innerGrid": {
"md": 6
},
"validationGrid": {
"md": 6
}
}
Since I won’t have the opportunity to do another review today, and I’m leaving for holiday for 3 weeks, I’d suggest asking someone else to take a second look after updating the PR based on the feedback. 🙂
Nice catch, this leads to new refactor where the entire added breackPoints is removed now |
|
4 participants
Description
This PR improves
ComponentStructureWrapperby simplifying the inner/validation grid merging logic and making exact-width input behavior easier to read. bymergeMaxGridSizePairandmergeMaxGridStyling.It also adds
validationGridto the shared generated grid type incodegen/common.ts;For now the validation messages use grid.validationGrid and when it’s missing, it falls back to innerGrid.
Related Issue(s)
Test in ra0479-02;
Screen.Recording.2026-03-26.at.10.18.29.mov
Test in forntend-test app:
Screen.Recording.2026-03-26.at.10.14.23.mov
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Refactor
Tests