Skip to content

Conversation

@gu-stav
Copy link
Contributor

@gu-stav gu-stav commented Mar 16, 2022

Before this PR the actual input element didn't cover the full height of it's wrapper. This leads to the problem that you can not click everywhere since the click might end up in the wrapper.

This PR removes height from the wrapper and bumps to a height of the input elements to the same as the container by using padding. It is not possible to use height on input fields - therefore I had to use some magic numbers.

Before After
Screenshot 2022-03-16 at 14 18 00 Screenshot 2022-03-16 at 14 18 13

Demo

@vercel
Copy link

vercel bot commented Mar 16, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

design-system – ./

🔍 Inspect: https://vercel.com/strapijs/design-system/2pr45Dqnf8Ue3eBGsAn8UEpJCWU4
✅ Preview: https://design-system-git-fix-field-input-height-strapijs.vercel.app

design-system-website – ./website

🔍 Inspect: https://vercel.com/strapijs/design-system-website/47b69cDHtZvpJsj29AoPZBAmdvwQ
✅ Preview: https://design-system-website-git-fix-field-input-height-strapijs.vercel.app

Copy link
Collaborator

@HichamELBSI HichamELBSI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks.

@HichamELBSI HichamELBSI added the pr: enhancement This PR adds or updates some part of the codebase or features label Mar 16, 2022
@HichamELBSI HichamELBSI added this to the 1.1.0 milestone Mar 16, 2022
Copy link
Contributor

@ronronscelestes ronronscelestes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, will try to find a proposal about how we can handle it but not using getThemeSize also means that we lose the S size documented in the DS system 😰

@gu-stav
Copy link
Contributor Author

gu-stav commented Mar 16, 2022

@ronronscelestes Ah, I didn't see how/ if the small size would be affected by that. If so (I'll double-check tomorrow), we could just reduce the padding for that size I guess. Or do you have something different in mind?

There is basically no way to set an explicit height on input fields - it always has to be achieved by a combination of line-eight, font-size and padding ...

@ronronscelestes
Copy link
Contributor

ronronscelestes commented Mar 16, 2022

@gu-stav interesting didn't know that!
But then don't we have the same issue with other components using getThemeStyle['input'] (like ToggleCheckbox and Select)?

Then you what do you think about keeping the getThemeStyle['input'] but with padding values, just to ensure we are using theme sizes? If we want to make a change one day it will be passed on to all "inputs" components using it (aka ToggleCheckbox and Select).
In case I'm not clear, something like:

export const sizes = {
  input: {
    S: `${value fitting for S size / 16}rem`,
    M: `${10,5 / 16}rem`,
  },
  accordions: {
    S: `${48 / 16}rem`,
    M: `${88 / 16}rem`,
  },
};

@gu-stav
Copy link
Contributor Author

gu-stav commented Mar 17, 2022

@ronronscelestes I've fixed size=S - thanks again. Very good catch!

For now, I'd keep the padding definition local, because I don't think we should advertise it since all other components use an explicit height. It works for them because they aren't native input fields.

I'd consider this a fix and not a methodology. If we would put it into a general schema/ function, we'd need to find a good name for it and start to advertise its usage. I'd therefore keep it local to the component if it's ok for you.

@gu-stav gu-stav requested a review from ronronscelestes March 17, 2022 09:20
@ronronscelestes
Copy link
Contributor

hey @gu-stav
thanks for the fix!

tbh I'm not 100% fan we remove usage of sizes object for the inputs, that would be nice indeed to find a more global way to handle this.
maybe we could at least add a card in the DS backlog to make sure we don't forget it? WDYT @HichamELBSI?

@HichamELBSI
Copy link
Collaborator

Can we merge this one @gu-stav ?

@gu-stav
Copy link
Contributor Author

gu-stav commented Mar 28, 2022

@HichamELBSI from my side: yes. @ronronscelestes was not happy with the implementation and asked for your feedback. That's the reason why I kept it open.

@HichamELBSI
Copy link
Collaborator

HichamELBSI commented Mar 28, 2022

Let's merge this PR and create a card in the DS backlog to not forget how we can handle this globally. @gu-stav @ronronscelestes

@HichamELBSI HichamELBSI merged commit 5df2ea5 into main Mar 28, 2022
@HichamELBSI HichamELBSI deleted the fix/field-input-height branch March 28, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: enhancement This PR adds or updates some part of the codebase or features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants