Skip to content
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

Add inputHeight prop #886

Closed
wants to merge 1 commit into from
Closed

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Dec 6, 2017

This allows for complete customization of the inputHeight via prop. It also cleans up some logic that was introduced with the verticalSpacing prop (although I'm not counting the deletion of the getInputHeight method as breaking because that change had not yet been released).

I was considering changing the theme props related to input and displaytext padding ... but I'm not sure that it is necessary? They do technically apply but I guess they are not as meaningful? I would take thoughts on that.

to: @gabergg @ljharb @erin-doyle @noratarano @airbnb/webinfra

@majapw majapw added the semver-minor: new stuff Any feature or API addition. label Dec 6, 2017
@majapw majapw force-pushed the maja-add-customizable-input-height branch from 724ec38 to 4eafb44 Compare December 6, 2017 22:59
@coveralls
Copy link

Coverage Status

Coverage decreased (-81.7%) to 5.014% when pulling 4eafb44 on maja-add-customizable-input-height into be5e24f on master.

font: { input: { lineHeight } },
spacing: { inputPadding, displayTextPaddingVertical },
} = reactDates;
const inputHeight = getInputHeight({ lineHeight, inputPadding, displayTextPaddingVertical });
Copy link
Member

Choose a reason for hiding this comment

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

i'm confused; how can all this logic be replaced by just passing down a prop? I'm fine with it being overrideable, but wouldn't the default need this same logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really because once you set the height to a fixed value, this padding doesn't affect it anymore. Although I guess that the displayTextPadding might still increase the whole thing... it's not really being used anymore, so this might be an opportunity just to axe it (it's kind of a weird pattern that doesn't make terribly much sense now that we have a real input).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, I'm definitely wrong. Let me reinvestigate!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 86.653% when pulling 4eafb44 on maja-add-customizable-input-height into be5e24f on master.

@majapw
Copy link
Collaborator Author

majapw commented Dec 6, 2017

@ljharb so (1) I think I'm going to get rid of the inputPadding theme variable because it doesn't make sense to have both that and the displayTextPadding with the new styling/structure. I will probably rename displayTextPadding as inputPadding and apply it to the actual input component. So the structure of DateInput will look like so [highly simplified]:

<div {...css(..., { height: props.inputHeight })}>
  <input {...css(..., { padding: theme.spacing.inputPadding })} />
</div>
  1. inputHeight is the one true way to set height, set overflow: 'hidden' on the wrapping div. You can still adjust the padding but it doesn't affect the final height.

  2. The inputHeight prop sets the height css variable but you can still affect things with the padding. This requires bringing back the getInputHeight util which does something like max(height, 2*padding.

I kind of prefer 1... but idk if that's oversimplifying.

@majapw
Copy link
Collaborator Author

majapw commented Jan 17, 2018

Closing! Don't think this is a level of customization we are going to pursue.

@majapw majapw closed this Jan 17, 2018
@majapw majapw deleted the maja-add-customizable-input-height branch January 17, 2018 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor: new stuff Any feature or API addition.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants