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

ebayui 812 & 813 - dropdown options UI fixes #736

Merged
merged 4 commits into from
Aug 6, 2019

Conversation

msendlakowski
Copy link
Contributor

@msendlakowski msendlakowski commented Aug 3, 2019

Description

Added background color to DS6 focused dropdown options.

Buttons content is naturally centered. When used in a dropdown the text needs to be aligned left.

Context

For issue 812, I referenced the base dropdown mixin from the DS specific dropdown mixins, and then added the override.

References

eBay/ebayui-core#812
eBay/ebayui-core#813

Screenshots

Screen Shot 2019-08-02 at 5 07 54 PM

Screen Shot 2019-08-02 at 5 42 57 PM

@msendlakowski msendlakowski changed the title ebayui 813 - align dropdown options left when rendered as button ebayui 812 & 813 - dropdown options UI fixes Aug 3, 2019
@ianmcburnie
Copy link
Contributor

ianmcburnie commented Aug 5, 2019

I believe I removed the background colour in order to fix a keyboard focus issue in the non-dropdown version, may be relevant here, let me dig up the commit...

Here: 27f1e5d

Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

I looked at the old changes, this was missing from menu

button.fake-menu__item {
    background-color: @dropdown-fake-button-background-color;
    border: 0;
    color: @dropdown-fake-button-color;
    display: block;
    font-family: inherit;
    text-align: left;
}

So I would add this whole section as

button.fake-menu-button__item {
    background-color: @dropdown-fake-button-background-color;
    border: 0;
    color: @dropdown-fake-button-color;
    display: block;
    font-family: inherit;
    text-align: left;
}

@msendlakowski
Copy link
Contributor Author

I believe I removed the background colour in order to fix a keyboard focus issue in the non-dropdown version, may be relevant here, let me dig up the commit...

Here: 27f1e5d

@ianmcburnie I think I addressed this by only applying the background color when the item is focused. But I'm not quite sure what you mean in the referenced commit about "non overlay state"

@ianmcburnie
Copy link
Contributor

It's a little confusing, because some of these rules are being used not just in the dropdown/overlay version (i.e. menu-button, listbox-button), but also in the version that is not in a dropdown/overlay (i.e. menu, listbox). Some of these mixins need to be renamed.

@ianmcburnie
Copy link
Contributor

Aha, @agliga's comment makes sense. If that entire block is missing (probably due to bad copy & paste on my part) then just restoring it is the right thing to do.

@msendlakowski
Copy link
Contributor Author

Oh. I see. The other missing button styles aren't being seen as an issue because our test page defaults aren't exposing them. The values from the user agent is visibly the same as the overrides. Only the text-align is a visible problem on our tests.
@ianmcburnie @agliga Out of the 6 styles Andrew shared above, border and display don't look like they apply anymore. border for the link and button is already 1px solid #fff and display for the link and button is already inline-flex.

Are you OK if I do:

button.fake-menu-button__item {
    background-color: @dropdown-fake-button-background-color;
    color: @dropdown-fake-button-color;
    font-family: inherit;
    text-align: left;
}

@msendlakowski msendlakowski merged commit 22027ed into 8.0.0 Aug 6, 2019
@msendlakowski msendlakowski deleted the 813-ebayui-dropdown-option branch August 6, 2019 00:10
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.

4 participants