Create Form and TextInput components #33
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Disclaimer: This comment was AI-generated and is not necessarily completely accurate. Please take code comments with a grain of salt.
PR Summary
This PR introduces new BogForm and BogTextInput components with their associated styles, implementing form functionality using Radix UI primitives and consistent styling patterns.
- BogTextInput is missing 'search' type option in the type prop definition
- Default value for
requiredprop in BogTextInput should be false per requirements - Form.Message components in BogTextInput may cause layout issues being placed next to label
- Style and className props are defined but unused in BogTextInput component
PR Score: 85/100
Checklist items needing attention:
Frontend
- Uses appropriate hooks only when necessary
- Uses relative positioning instead of absolute where appropriate
The implementation is solid overall but needs minor adjustments to fully meet requirements and maintain consistent styling patterns.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
5 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| &:focus { | ||
| outline: none; | ||
| border-color: var(--color-brand-stroke-strong); | ||
| } |
There was a problem hiding this comment.
style: Consider adding a focus ring or box-shadow for better accessibility and visual feedback on focus
| &:focus { | |
| outline: none; | |
| border-color: var(--color-brand-stroke-strong); | |
| } | |
| &:focus { | |
| outline: none; | |
| border-color: var(--color-brand-stroke-strong); | |
| box-shadow: 0 0 0 2px var(--color-brand-stroke-weak); | |
| } |
There was a problem hiding this comment.
Yeah, this isn't in the Figma, but would you mind adding this. Obviously, not your mistake, I just think it looks better and it's something the designers should have put in initially.
| onSubmit={(e) => { | ||
| e.preventDefault(); | ||
| const data = Object.fromEntries(new FormData(e.currentTarget)); | ||
| console.log(data); |
There was a problem hiding this comment.
style: Remove console.log before production
There was a problem hiding this comment.
Yeah, thanks for leaving this behind for testing, but can you get rid of the log? If possible, could the event handler for the form on this page just be to clear all inputs on the form?
rgodha24
force-pushed
the
rohan/31-form-component
branch
from
cc10739 to
cbf7423
Compare
| Please provide a valid {label} | ||
| </Form.Message> | ||
| </div> | ||
| <Form.Control asChild> |
There was a problem hiding this comment.
I'd like a little bit more vertical space between the stuff above the text input (label and messages) and the textbox itself please :)
| placeholder={placeholder} | ||
| required={required} | ||
| disabled={disabled} | ||
| className={`${styles.input} text-paragraph-2`} |
There was a problem hiding this comment.
Can you style the placeholder text on both of these inputs like the Figma? I think there is a ::placeholder CSS selector, and it would just be setting the font to text-paragraph-2 and the color to var(--color-grey-stroke-strong)
| border-color: var(--color-brand-stroke-strong); | ||
| } | ||
| &:disabled { | ||
| color: var(--color-grey-off-state); |
There was a problem hiding this comment.
For disabled, can we make the hover state not change the border and the cursor be 'not-allowed'
closes #31