Repository navigation
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
coverage: 84.866% (+0.06%) from 84.803% — jgteske:feat-react-material-adornments into eclipsesource:master |
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
This PR adds startAdornment / endAdornment UI schema options (plain strings) to the React Material text, number, integer, number-format, select, autocomplete and anyOf-string-or-enum inputs (closes #2018). A new useInputAdornments hook in util/adornment.tsx builds MUI InputAdornments and sets notched for the outlined variant. MaterialInputControl force-shrinks the label when a start adornment is set, because MUI can't detect the adornment through the wrapped inner component. Existing built-in end adornments (clear button, autocomplete popup icons, anyOf datalist) are merged with the user adornment, not replaced. The PR also adds an example and a test suite.
The integration is consistent. Every Material input reads the options the same way, and the existing behaviour without the options is unchanged (the clear button logic is equivalent when no end adornment is set). Points for maintainers:
- Clear button layout in
MuiInputText: when an end adornment is set, the clear button permanently reserves space, which leaves a visible gap (inline comment). - Public API naming: the hook,
isAdornmentTextand the new types become public API through the util re-export. One of the types has the same name as a MUI type (inline comment). - Label shrink vs.
notched: the two are handled in different places (MaterialInputControlvs. the hook). They have to stay in sync for the outlined notch to look right. - Example scope: the new "Adornments" example is shared, so it shows up in all renderer sets, but only React Material supports the options.
I couldn't do UI verification: the review sandbox has no network access, so the deploy preview couldn't be loaded and dependencies couldn't be installed for a local build. A maintainer should check the visual result in the deploy preview, especially the select arrow offset (mr: 4) and the outlined label notch.
|
Hi @jgteske , thank you for the contribution ❤️ |
|
Hi @lucas-koehler, |
4548711 to
cce74a7
Compare
lucas-koehler
left a comment
lucas-koehler
left a comment
There was a problem hiding this comment.
Hi @jgteske ,
thanks for the updates! I have some inline comments. Besides that we should consider the placement of the endadornment vs control buttons again:
IMO the clear button, which only shows up on hover, should be left of the end adornment to avoid leaving a blank gap while it is not shown. For instance, see the current state of the autocomplete control in the adornments example:
no hover:
hovered:
The dropdown arrow should stay on the right end in my opinion. The same should be checked for select controls. In the example, they are not clearable. Please check if they can be clearable.
In general, the order on the right should be:
clear - end adornment - other buttons (dropdown arrow)
If that is too complicated, having the end adornment completely on the right (after the other buttons) would be fine, too. It should be consistent over all affected renderers though.
| } from '../util'; | ||
|
|
||
| // MUI positions the dropdown arrow absolutely, so it would cover the end adornment otherwise | ||
| const adornmentSx: InputAdornmentSx = { endAdornmentSx: { mr: 4 } }; |
There was a problem hiding this comment.
Same ordering issue as in the autocomplete: the dropdown arrow should sit before the suffix, like the clear button in the text field.
Also, mr: 4 goes through theme.spacing. With createTheme({ spacing: 4 }) it resolves to 16px, and the outlined/filled arrow (right: 7px, 24px wide) then overlaps the adornment. A fixed px value avoids that (the standard variant has the arrow at right: 0, so it differs per variant anyway).
There was a problem hiding this comment.
I noticed a small inconsistency between this issue and the generally described Autocomplete issues:
one requests clear - suffix - dropdown arrow (Autocomplete), while the other asks for dropdown arrow - suffix (Select).
MUI’s Autocomplete customization guidance recommends preserving the supplied input props and built-in adornments when adding custom content. Following that principle, I recommend keeping the standard controls together for the AutoComplete and appending the suffix here as well to make it consistent with the Select: clear - dropdown arrow - suffix. This will also make the implementation simpler and less likely to break in the future.
|
Hi @lucas-koehler, |
e479a6e to
289481d
Compare
Closes #2018