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

Borders and icons fix for Windows' high contrast mode #619

Merged
merged 9 commits into from
Sep 16, 2021

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Sep 13, 2021

Why are these changes introduced?

Fixes #408

What approach did you take?

Followed the suggestion from the issue. Using a media query specific to when high contrast mode is used.

Other considerations

Demo links

Checklist

Testing

  • Inputs,
  • Buttons,
  • Icons (social media, header, collapsible tab),
  • Table (customer account),
  • loading spinner in cart

The loading spinner for the add to cart is not coming up properly but it's being changed in another PR to remove the disabled attribute (#582).

@ludoboludo ludoboludo requested a review from svinkle September 13, 2021 15:06
@svinkle
Copy link
Member

svinkle commented Sep 13, 2021

@ludoboludo Looking good. A few things:

  1. Would it be possible to apply a transparent border for facet selectors?
  2. Remove facet x icon is still difficult to see
  3. Sort control border needs some padding
  4. Facet checkbox is visually the same state; checked or unchecked (set border instead of background-color in media query)
    @media screen and (forced-colors: active) {
      .facet-checkbox>svg {
        background-color: inherit; /* "remove" the background */
        border: solid 1px rgb(var(--color-background));
      }
      .facet-checkbox>input[type=checkbox]:checked~.icon-checkmark {
        border: none;
      }
    }
  5. Product selection is visually the same state; selected or unselected (add an underline)
    @media screen and (forced-colors: active) {
      .product-form__input input[type=radio]:checked+label {
        text-decoration: underline;
      }
    }
Screenshots

@ludoboludo ludoboludo force-pushed the high-contrast-mode-fixes branch from 1139dfd to 357d8d6 Compare September 14, 2021 15:00
Comment on lines 188 to 192
@media screen and (forced-colors: active) {
.collection-filters__sort {
border: none;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svinkle what about just removing the border overall ? So it has the same look as when it's not using windows' high contrast mode ?

svinkle
svinkle previously approved these changes Sep 14, 2021
@tauthomas01 tauthomas01 self-assigned this Sep 14, 2021
Copy link
Contributor

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for doing this, I have never seen these new properties and media query so this is interesting 👍

svinkle
svinkle previously approved these changes Sep 15, 2021
tauthomas01
tauthomas01 previously approved these changes Sep 15, 2021
Copy link
Contributor

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

LGTM

@ludoboludo ludoboludo dismissed stale reviews from tauthomas01 and svinkle via 62be7cb September 15, 2021 14:51
@ludoboludo
Copy link
Contributor Author

Sorry @svinkle & @tauthomas01, I just pushed one last thing I noticed. When adding to the cart, while the submit button had aria-disabled="true", it should be hiding the Add to cart text and showing only the loading spinner. But it was still showing the text under the spinner. With the latest commit it's fixed 👌

tauthomas01
tauthomas01 previously approved these changes Sep 15, 2021
svinkle
svinkle previously approved these changes Sep 16, 2021
@ludoboludo ludoboludo dismissed stale reviews from svinkle and tauthomas01 via 9f3a441 September 16, 2021 18:43
@ludoboludo
Copy link
Contributor Author

Annnnd same thing again. Facet filter on search page got merged so it created some conflicts I had to fix. Sorry for the re request. 😬
@svinkle @tauthomas01

@svinkle
Copy link
Member

svinkle commented Sep 16, 2021

LGTM

Copy link
Contributor

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

Facets work well

@ludoboludo ludoboludo merged commit 3bf1970 into main Sep 16, 2021
@ludoboludo ludoboludo deleted the high-contrast-mode-fixes branch September 16, 2021 19:15
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
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.

[Global] Icons and borders are not visible in Windows High Contrast mode
4 participants