Skip to content

Conversation

@LauraAubin
Copy link
Contributor

WHY are these changes introduced?

Part of #795

Border is missing on high contrast mode in Windows.

WHAT is this pull request doing?

Adds a transparent border to the modal so that in high contrast we can see the modal outline.

Couldn't screenshot since I don't have Windows setup.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@LauraAubin
Copy link
Contributor Author

@dpersing could you tophat this change on Windows with high contrast mode? I don't have Windows setup yet 🙏

@alex-page
Copy link
Member

@dpersing could you tophat this change on Windows with high contrast mode? I don't have Windows setup yet 🙏

Let me know if you want help setting this up!

box-shadow: shadow(layer);

@media (-ms-high-contrast: active) {
border: border(transparent);

Choose a reason for hiding this comment

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

For this line, we'll want to use an actual color instead of transparent. Basically, there are two ways to make a border show up:

  • Use a transparent border by default in regular CSS
  • Use a colored border in the @media query

Calling for a transparent border with the @media query actually displays a transparent border! Which seems weird, but there we are.

No border visible in high contrast

(Please ignore the highlighted text; that's macOS and VMWare not playing nice together while I tried to take a screenshot.)

In this case, I think we might want to go with the default foreground text color for High Contrast, which would look like something like this:

@media (-ms-high-contrast: active) {
    border: 1px solid ms-high-contrast-color('text');
  }

Solid default border using the foreground color in High Contrast

There are some other examples of that usage in the codebase that might help.

Let me know too if I can help you get set up with test tools!

Copy link
Member

@chloerice chloerice Sep 11, 2019

Choose a reason for hiding this comment

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

@LauraAubin your hunch was right! @dpersing the reason we went with the media query was to be declarative about why the border is there, so the solution you mentioned above is :chefkiss:

@dpersing dpersing self-requested a review September 11, 2019 18:17
Copy link

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

Sweeeeet. Let me know if I can help further, @LauraAubin and @chloerice !

@dpersing
Copy link

@LauraAubin The color is showing up correctly in dev tools now, but it's not displaying because there's no width set! Should just need a 1px solid or similar:

border: 1px solid ms-high-contrast-color('text');

Screenshot showing the border color being set, but not displaying

I think the closest example is the one that's in the Progress bar, where the border and the color is being set. Let me know if that helps!

@LauraAubin
Copy link
Contributor Author

Setting up my windows VM so I can actually check these changes haha 😅

@LauraAubin LauraAubin force-pushed the modal-high-contrast-border branch from a7cf7d0 to e4681e1 Compare September 12, 2019 18:23
@LauraAubin
Copy link
Contributor Author

My border color doesn't show up as yellow but I think that's just my VM.

Screen Shot 2019-09-12 at 2 22 07 PM

@LauraAubin LauraAubin force-pushed the modal-high-contrast-border branch 2 times, most recently from 41a35b5 to 23e3814 Compare September 12, 2019 18:27
@LauraAubin LauraAubin force-pushed the modal-high-contrast-border branch from fc6acd5 to 0d5ea7d Compare September 12, 2019 18:27
@dpersing
Copy link

My border color doesn't show up as yellow but I think that's just my VM.

It depends on the High Contrast theme you pick! Looks perfect. 🎉

@dpersing dpersing self-requested a review September 12, 2019 20:06
Copy link

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

Yay for getting your VM set up! This looks great.

@LauraAubin LauraAubin merged commit cd3cf26 into master Sep 13, 2019
@LauraAubin LauraAubin deleted the modal-high-contrast-border branch September 13, 2019 13:36
@dleroux dleroux temporarily deployed to production September 20, 2019 17:44 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:28 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:38 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 16:59 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 17:05 Inactive
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.

6 participants