-
Notifications
You must be signed in to change notification settings - Fork 93
LF-5028 Refactor <Input /> to use <InputBaseLabel /> and adjust Optional styles #3913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LF-5028 Refactor <Input /> to use <InputBaseLabel /> and adjust Optional styles #3913
Conversation
…ails views Inconsistency with other radios through app was more annoying than inconsistency with other inputs in the same form. I did remove 4px of margin as it was just too much. However, note that other radios in app still have that massive margin from their own labels -- this I did not correct
| hasLeaf={hasLeaf} | ||
| toolTipContent={toolTipContent} | ||
| icon={icon} | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As usual, changing the wrapping divs has made a messy diff! The changes are these two commits:
- refactor Input to use InputBaseLabel, and
- wrap all items rendered within input in .inputWrapper. This moves the password icon, currency symbol, and search icon into the
.inputWrapperdiv for positioning
| @@ -1,5 +1,20 @@ | |||
| @import '@assets/mixin.scss'; | |||
|
|
|||
| .container { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change to the .container class; just put at the top of the file for easier reading. .inputWrapper was also re-positioned at the top of the file and gained these two properties:
display: flex;
align-items: centerLabel styles + now unnecessary vertical positioning styles were removed.
| optional={true} | ||
| labelStyles={{ | ||
| marginBottom: '12px', | ||
| fontSize: '16px', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rather bothered that the radio group labels are larger with greater margin than the other form labels in app, but there are many, many more such labels (without optional property) not touched in this PR, so I went with consistency with the other radio groups here, rather than with the other form elements. Personally I think there is room to refactor all of these, but likely not a priority.
…a, and InputAutoSize
| position: relative; | ||
| } | ||
| align-items: baseline; | ||
| margin-bottom: 4px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to never set a margin on a component (and let the parent / layout component control spacing), but <InputBaseLabel /> has been used so many times in app relying on the 4px margin that the interior <Label /> elements provide that refactor would be a beast; so I kept it.
Here I am setting the margin on the entire label section and over-writing the margin of the two interior <Label /> components, though -- this is to make sure the other label elements like the <PrivateBadge /> get the correct margin from the <input> element as well, and that the main label and optional text don't get more. It's a finicky part of the styling, because of the margins that got set on the primitive components. Not sure it's 100% ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Love to see that reusability! Only really blocking on the missing translation string.
Condensed forms it does look better. But I did personally kind of like optional where it was before, on desktop it is now harder to tell what the optional applies too -- you have to follow the input across the screen haha.
Other:
- Those existing location details files always make me sad to see all the duplication haha
- While we are have a Litefarm label expert does it make sense to you to fix our accessibility? Specifically the clicking on the label behaviour. It works on mobile that when you click it -- it focuses the input. But not desktop.
- onLabelClick => focus input -- especially useful for desktop
- Special case Checkbox (see Add animals) => label currently toggles instead of focuses (info box is part of label and also toggles on click)
- Open question does leaf and info part of label == focus /select?
| const { t } = useTranslation(); | ||
| return ( | ||
| <div className={styles.private}> | ||
| <span>{t('MARKET_DIRECTORY.INFO_FORM.PRIVATE')}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this string exists yet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HA! It was capitalized in the UI and the same string so I didn't notice 😂 Thank you!!
Edit: Oh wait, what am I talking about 😓 It's just in the form part of the translations which I have local but haven't committed. I'll put it in a more general place
packages/webapp/src/components/LocationDetailLayout/AreaDetails/Barn/index.jsx
Show resolved
Hide resolved
| stars: 5, | ||
| }; | ||
|
|
||
| export const Optional = Template.bind({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of any optional ratings... but I am wondering if it is an exception like the signup flow radio -- it is hard to decipher on the other end of the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! I think the cleanest way to handle would be to make it a prop on the label, and keep it on the left for any elements that don't extend across the screen. I'll do this for radios and rating... Oh, and I see this one doesn't use the component so I'll copy the non-component radios pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and I just found the optional rating in app! It was in management plan:
<Rating
stars={value}
onRate={onChange}
style={{ marginBottom: '40px' }}
disabled={completed || abandoned}
optional
label={t('MANAGEMENT_PLAN.COMPLETE_PLAN.RATING')}
/>
packages/webapp/src/components/SimpleBadges/PrivateBadge/index.tsx
Outdated
Show resolved
Hide resolved
|
Thanks @Duncan-Brain! String and positioning should be fixed. As for the accessibility refactor (correct use of As for the point about leaf/optional -- should be included so it's read by screen reader as well. Not sure they have any alt text set up for that yet though -- will have to investigate 😬 |
Description
A much more sizeable refactor than I had hoped! Since so many files were touched (although each diff should be pretty easy to understand), I'll do a more complete summary/review guide below.
Summary of changes:
<InputBaseLabel />component to display its right-hand objects (Optional, tooltip, icon) in a flex container<Input />to use<InputBaseLabel />instead of duplicating styles<Input />to follow something a lot more like<InputBase />, where all the input interior components (search icon, visibility icon, currency, etc) are flex items within the flex wrapper.<TextArea />didn't have the optional prop; added. Also fixed the focus style while I was in the fileTo update through the application, I separated the use of Label into two categories:
Category A: Optional + other Label modifiers
Re-useable components OR individual form elements (Location Details forms) with at least one other modifier (leaf/tooltip) alongside optional
For all of these components (Unit) and forms (Greenhouse, Fence), I imported and used
<ImportBaseLabel />.Category B: Only optional modifier
Re-useable components OR individual form elements (Location Details forms) where the only label modifier was "optional".
Here the larger
<InputLabelBase />component is not necessary. Where possible I tried to harmonize the pattern with a shared stylesheet, but the core was just a very small bit of JSX + CSSThere was exactly ONE
"optional"in app I didn't touch, and that is in this view:This one just doesn't look like a form (one radio group's label is the page title?!), and I thought the optional would look quite weird floating alone on the right side of the page.
Note: I will probably double-check with Loïc tomorrow about Radio Group label optionals in general though, and make sure they were meant to be moved 🙂
UPDATE: Loïc said all elements that don't extend across the view should have optional cuddled to the label, so I have moved it back for the radio button labels and
<Rating />(did not revert the actual changes though as the unified styles for Radio labels are an improvement, and should make it easier to refactor in the future).Jira link: https://lite-farm.atlassian.net/browse/LF-5028
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)