Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Fixed modal default focus issue #3

Merged
merged 5 commits into from
Oct 17, 2018

Conversation

blackbaud-conorwright
Copy link
Contributor

Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Looks like you have a visual test failing, but otherwise looks good!

@codecov-io
Copy link

codecov-io commented Oct 5, 2018

Codecov Report

Merging #3 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
+ Coverage   99.75%   99.76%   +<.01%     
==========================================
  Files          19       19              
  Lines         416      420       +4     
  Branches       64       65       +1     
==========================================
+ Hits          415      419       +4     
  Misses          1        1
Impacted Files Coverage Δ
...c/modules/modal/modal-component-adapter.service.ts 100% <100%> (ø) ⬆️

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 82854ae...747fa01. Read the comment docs.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

@blackbaud-conorwright Do you mind adding a visual test that demonstrates an input with autofocus working? I know we have one for the unit tests, but it'd be nice to get a screenshot with the focus blur on an <input type="text" autofocus>.

@blackbaud-conorwright
Copy link
Contributor Author

Added an autofocus visual test. I also added names for all the screenshots, because adding a new visual test pushed them each down a number and would have caused me to regenerate baselines that weren't directly affected (because they now matched screenshot 18 rather than 17)

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Just one more issue.

Also: If you add z-index: 1; to the .sky-modal-content style, it will make the blue focus blur appear on top of the modal footer.

@Blackbaud-AlexKingman Blackbaud-AlexKingman dismissed Blackbaud-SteveBrush’s stale review October 17, 2018 12:46

Steve's comment (z-index) has been seen to. Dismissing to add final approval.

Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Ship it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants