Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Adjust form control line height to fit descenders #261

Merged
merged 8 commits into from
Apr 22, 2021

Conversation

johnhwhite
Copy link
Member

No description provided.

@johnhwhite johnhwhite self-assigned this Apr 19, 2021
@blackbaud-ado
Copy link
Member

@blackbaud-ado
Copy link
Member

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #261 (b9aaca4) into master (ef722d5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #261   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines         1060      1060           
  Branches       202       202           
=========================================
  Hits          1060      1060           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edbd370...b9aaca4. Read the comment docs.

@johnhwhite johnhwhite marked this pull request as ready for review April 19, 2021 17:48
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Looks good, pending UX approval.

@Blackbaud-ToddRoberts
Copy link

@Blackbaud-AdamFunderburk do we need to do anything to compensate for this making input boxes 57px tall rather than 55px?

@blackbaud-ado
Copy link
Member

@Blackbaud-ToddRoberts
Copy link

@Blackbaud-TrevorBurch will the change in input box height this causes lead to any issues/inconsistencies with superselect/lookup?

@Blackbaud-AdamFunderburk
Copy link
Contributor

@Blackbaud-AdamFunderburk do we need to do anything to compensate for this making input boxes 57px tall rather than 55px?

I checked with @Blackbaud-TrevorBurch about the position of Tokens inside an Input Box and it looks like we'll need to move them down 1px to stay aligned with the new value text.

We saw some differences in the position of the new value text and placeholder text. Is this new line height supposed to be applied to placeholder text as well?

@blackbaud-ado
Copy link
Member

@blackbaud-ado
Copy link
Member

@blackbaud-ado
Copy link
Member

@johnhwhite
Copy link
Member Author

@Blackbaud-AdamFunderburk do we need to do anything to compensate for this making input boxes 57px tall rather than 55px?

I checked with @Blackbaud-TrevorBurch about the position of Tokens inside an Input Box and it looks like we'll need to move them down 1px to stay aligned with the new value text.

We saw some differences in the position of the new value text and placeholder text. Is this new line height supposed to be applied to placeholder text as well?

@Blackbaud-ToddRoberts @Blackbaud-AdamFunderburk I've adjusted the padding to compensate for the height and that reduced the number of visual regression issues. HTML placeholder should pick up the style adjustment.

I'm checking with @Blackbaud-TrevorBurch on the tokens issue.

@blackbaud-ado
Copy link
Member

@@ -1,6 +1,7 @@
@import "~@skyux/theme/scss/mixins";
@import "~@skyux/theme/scss/_compat/mixins";
@import "~@skyux/theme/scss/variables";
@import "~@skyux/theme/scss/_compat/variables";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this import necessary?

@johnhwhite johnhwhite merged commit bc9317f into master Apr 22, 2021
@johnhwhite johnhwhite deleted the input-line-height branch April 22, 2021 19:02
@johnhwhite johnhwhite mentioned this pull request Apr 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants