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

feat(HighContrast): Expose the XAML HighContrastChanged event to JS #1443

Merged
merged 3 commits into from
Oct 31, 2017

Conversation

antonkh
Copy link
Contributor

@antonkh antonkh commented Oct 12, 2017

The event is exposed as highContrastDidChange in AccessibilityInfo. For now
it only tells if high contrast is enabled or disabled in system settings. Later it
can tell what are the chosen high contrast colors: Windows 10 allows to create
custom high contrast themes with user chosen colors.

The event is exposed as highContrastDidChange in AccessibilityInfo. For now
it only tells if high contrast is enabled or disabled in system settings. Later it
can tell what are the chosen high contrast colors: Windows 10 allows to create
custom high contrast themes with user chosen colors.

public override void Initialize()
{
_accessibility.HighContrastChanged += OnHighContrastChanged;
Copy link
Contributor

Choose a reason for hiding this comment

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

_accessibility.HighContrastChanged += OnHighContrastChanged; [](start = 12, length = 60)

Should we do this in Initialize or in the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eric suggested to use Initialize.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. What's the reasoning behind preferring Initialize over the constructor? When does Initialize get called relative to Constants?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Constructor is called before the JS bundle is loaded, so you could potentially notify event to JS before anything there to listen. If you call after Initialize, bundle load operation has already been queued.


In reply to: 144430206 [](ancestors = 144430206)

Copy link
Contributor

Choose a reason for hiding this comment

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

Firing an event before anything is listening sounds pretty harmless.

So it sounds like it doesn't matter too much whether you register for the event in Initialize or in the constructor. Do you agree?


In reply to: 145428282 [](ancestors = 145428282,144430206)

return "rgba(" + color.R + "," + color.G + "," + color.B + "," + color.A + ")";
}

private IDictionary<string, string> GetHighContrastColors()
Copy link
Contributor

Choose a reason for hiding this comment

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

private IDictionary<string, string> GetHighContrastColors() [](start = 8, length = 59)

Maybe put a TODO about how we want to expose these colors to JavaScript in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

return new Promise((resolve, reject) => {
reject('AccessibilityInfo is not supported on this platform.');
resolve(false);
Copy link
Contributor

@rigdern rigdern Oct 12, 2017

Choose a reason for hiding this comment

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

resolve(false); [](start = 6, length = 15)

Can you put a comment about how UWP doesn't provide any way to query whether or not a screen reader is being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@rigdern rigdern left a comment

Choose a reason for hiding this comment

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

:shipit:

@codecov-io
Copy link

codecov-io commented Oct 12, 2017

Codecov Report

Merging #1443 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1443      +/-   ##
==========================================
- Coverage   31.82%   31.82%   -0.01%     
==========================================
  Files         263      263              
  Lines       19078    19078              
  Branches     1589     1589              
==========================================
- Hits         6072     6071       -1     
  Misses      12855    12855              
- Partials      151      152       +1
Impacted Files Coverage Δ
...tNative.Shared/Modules/Network/NetworkingModule.cs 68.08% <0%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b97ad28...6fe657f. Read the comment docs.

return "rgba(" + color.R + "," + color.G + "," + color.B + "," + color.A + ")";
}

// TODO: These are the 8 high contrast colors in Windows. Eventually they will be exposed to JS.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you file an issue for this TODO item?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: #1459

Copy link
Collaborator

@rozele rozele left a comment

Choose a reason for hiding this comment

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

:shipit:

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.

5 participants