Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-form-select] - Fix select option width and styling #4023

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

PK106552
Copy link
Contributor

@PK106552 PK106552 commented Feb 7, 2024

Summary

What was changed:

  1. Changed the --terra-form-select-option-padding variable value to the orion-fusion-theme to match consistent padding so that the option name doesn't break to the next line.
  2. Changed onHover style.

Why it was changed:

  1. The padding variable value in the orion-fusion-theme is bigger than in the other themes and causes the option name to break to the next line in the middle of the word even if the option is the same as current option.
  2. The onHover styling for the option looks rather like onFocus (dashed border), focus states cannot be the same as the hover states.

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-10121


Thank you for contributing to Terra.
@cerner/terra

Befor fix :

Screenshot 2024-02-07 at 10 11 31 AM Screenshot 2024-02-07 at 10 13 31 AM

After fix :

Screenshot 2024-02-07 at 10 10 34 AM Screenshot 2024-02-07 at 10 13 10 AM

@@ -2,6 +2,10 @@

## Unreleased

* Changed
* Changed --terra-form-select-option-padding value to the orion-fusion-theme to match consistent padding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Changed --terra-form-select-option-padding value to the orion-fusion-theme to match consistent padding.
* Changed --terra-form-select-option-padding value to match with the orion-fusion-theme for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated - d000854

Comment on lines -278 to -297
/**
* Sets the hovered option as the active value.
* @param {event} event - The mouse enter event.
* @param {ReactNode} option - The option that received the mouse enter event.
*/
handleMouseEnter(event, option) {
// Prevents setting the active option on mouse enter if the keyboard scrolled the view.
if (this.scrollTimeout) {
return;
}

if (!option.props.disabled) {
this.setState({ active: option.props.value });
}

if (option.props.onMouseEnter) {
option.props.onMouseEnter(event);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that this is now being handled by CSS but will this cause any loss of functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality will add only is-active class on mouse hover, now this being handled by CSS, and this won't cause any loss of functionality.

@PK106552 PK106552 requested a review from sdadn February 8, 2024 14:36
Copy link
Contributor

@adoroshk adoroshk left a comment

Choose a reason for hiding this comment

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

  1. Ensured that focus styling is now different from hover, both have background but only focus has dashed border in default, fusion and lowlight themes.
  2. Menu items padding fixed in fusion theme.
    LGTM
Screenshot 2024-02-08 at 9 48 22 AM

@PK106552
Copy link
Contributor Author

PK106552 commented Feb 8, 2024

  1. Ensured that focus styling is now different from hover, both have background but only focus has dashed border in default, fusion and lowlight themes.
  2. Menu items padding fixed in fusion theme.
    LGTM
Screenshot 2024-02-08 at 9 48 22 AM

Yes, focus styling is now different from hover, we have matched same onHover and onFocus style as other terra components.

@github-actions github-actions bot temporarily deployed to preview-pr-4023 February 23, 2024 16:16 Destroyed
@rahulkumar8cs
Copy link

+1

@sugan2416 sugan2416 added ⭐ Accessibility Reviewed Accessibility has been reviewed and approved. and removed Accessibility Review Required labels Feb 26, 2024
@sugan2416 sugan2416 merged commit f8a8dba into main Feb 26, 2024
22 checks passed
@sugan2416 sugan2416 deleted the form_select_issues branch February 26, 2024 06:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📦 terra-form-select ⭐ Accessibility Reviewed Accessibility has been reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants