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

Address height consistency between form elements #98

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

jordanjones243
Copy link
Contributor

@jordanjones243 jordanjones243 commented Sep 8, 2022

Alaska Airlines Pull Request

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Resolves: #81

Summary:

Please summarize the scope of the changes you have submitted, what the intent of the work is and anything that describes the before/after state of the project.

Add custom property $trigger-height to src/style.scss to allow for the height of select to be manipulated.

Type of change:

Please delete options that are not relevant.

  • New capability
  • Revision of an existing capability
  • Infrastructure change (automation, etc.)
  • Other (please elaborate)

Checklist:

  • My update follows the CONTRIBUTING guidelines of this project
  • I have performed a self-review of my own update

By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.

Thank you for your submission!

-- Auro Design System Team

@jordanjones243 jordanjones243 requested a review from a team as a code owner September 8, 2022 02:17
@jordanjones243 jordanjones243 self-assigned this Sep 8, 2022
@jordanjones243 jordanjones243 linked an issue Sep 8, 2022 that may be closed by this pull request
@jordanjones243 jordanjones243 force-pushed the jordanjones243/heightConsistency/#81 branch from fac95d2 to 1a6f3c2 Compare September 8, 2022 17:45
@blackfalcon blackfalcon changed the title Custom height CSS Property Address height consistency between form elements Sep 8, 2022
Copy link
Member

@blackfalcon blackfalcon left a comment

Choose a reason for hiding this comment

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

This issue really shouldn't have been a feature, but to address a bug.

Please update the commit message to something like:

fix(height): addjust to be consistent with other form elements #18

src/style.scss Outdated
@@ -2,6 +2,9 @@
@import './../node_modules/@alaskaairux/webcorestylesheets/dist/breakpoints';
@import './../node_modules/@alaskaairux/webcorestylesheets/dist/core';

/* Custom property to manipulate the height of auro-select element */
$trigger-height: 40px;
Copy link
Member

Choose a reason for hiding this comment

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

Recommend not using this. We are moving away from Sass variables as much as possible. I'd suggest creating local CSS variables.

:root {
  --trigger-height: 40px;
}

Ideally, we would want this to be governed by an Auro Design Token, but this will be addressed in a future update that is still in the works. AlaskaAirlines/AuroDesignTokens#71

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot create CSS variable in :root and use it in :host, therefore I created the variable in :host.

src/style.scss Outdated
Comment on lines 22 to 23
min-height: #{$trigger-height};
max-height: #{$trigger-height};
Copy link
Member

Choose a reason for hiding this comment

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

A couple of things here.

  1. When using Sass variables you do not need to use the #{} syntax. Referencing the $trigger-height variable itself would work.
  2. Let's update this to var(--trigger-height);

@jordanjones243 jordanjones243 force-pushed the jordanjones243/heightConsistency/#81 branch 2 times, most recently from e79a615 to b1abc48 Compare September 8, 2022 22:00
src/style.scss Outdated
Comment on lines 16 to 26
:host {
/* Custom property to manipulate the height of auro-select element */
--trigger-height: 40px;

auro-dropdown {
&::part(trigger) {
min-height: var(--trigger-height);
max-height: var(--trigger-height);
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to wrap this in a :host selector. This will work.

Suggested change
:host {
/* Custom property to manipulate the height of auro-select element */
--trigger-height: 40px;
auro-dropdown {
&::part(trigger) {
min-height: var(--trigger-height);
max-height: var(--trigger-height);
}
}
}
auro-dropdown {
--trigger-height: 40px;
&::part(trigger) {
min-height: var(--trigger-height);
max-height: var(--trigger-height);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@blackfalcon blackfalcon merged commit 92587e8 into v2.5-rc Sep 8, 2022
@blackfalcon blackfalcon deleted the jordanjones243/heightConsistency/#81 branch September 8, 2022 23:26
@DZwell
Copy link

DZwell commented Sep 8, 2022

👏 ❤️

@jordanjones243 jordanjones243 restored the jordanjones243/heightConsistency/#81 branch September 22, 2022 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the height of auro-select the same as auro-input
3 participants