Skip to content

October a11y bug fixes#2398

Merged
cRui861 merged 7 commits intomasterfrom
feature/a11y-oct-21
Dec 3, 2021
Merged

October a11y bug fixes#2398
cRui861 merged 7 commits intomasterfrom
feature/a11y-oct-21

Conversation

@cRui861
Copy link
Member

@cRui861 cRui861 commented Nov 11, 2021 •
edited
Loading

October accessibility bug fixes including fixes for focus order, focus visual indicator, role updates for screen reader, tooltip overlap on hover, and placeholder suggestions for editable fields.

@codecov
Copy link

codecov bot commented Nov 11, 2021 •
edited
Loading

Codecov Report

Merging #2398 (b6ac98e) into master (f733354) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b6ac98e differs from pull request most recent head 90cdd28. Consider uploading reports for the commit 90cdd28 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2398   +/-   ##
=======================================
  Coverage   65.25%   65.26%           
=======================================
  Files         903      903           
  Lines       26024    26025    +1     
  Branches     5115     5115           
=======================================
+ Hits        16983    16984    +1     
  Misses       9041     9041
Impacted Files Coverage Δ
...sk/ui/buttons/refresh-btn/refresh-btn.component.ts 81.57% <100.00%> (+0.49%) ⬆️

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 f733354...90cdd28. Read the comment docs.

@dpwatrous
Copy link
Member

dpwatrous commented Nov 11, 2021 •
edited
Loading

Looks good to me! Just a couple of minor suggestions as far as the actual Git commits are concerned:

  • Let's rebase this on master so we can use the 'create a merge commit' merge option and preserve individual commits for different a11y fixes. If we don't rebase we'll have 2 merge commits, which is going to look pretty confusing in Git history.

  • Since the DevOps work items aren't visible from GitHub, let's reword the commit messages with a brief description of each fix. You can still link the DevOps work item in the commit's description field. See my commit in this PR for an example.

dpwatrous
dpwatrous previously requested changes Nov 11, 2021
View reviewed changes
Copy link
Member

@dpwatrous dpwatrous left a comment

Choose a reason for hiding this comment

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

(See my comment above)

@cRui861
Copy link
Member Author

cRui861 commented Nov 11, 2021

Looks good to me! Just a couple of minor suggestions as far as the actual Git commits are concerned:

  • Let's rebase this on master so we can use the 'create a merge commit' merge option and preserve individual commits for different a11y fixes. If we don't rebase we'll have 2 merge commits, which is going to look pretty confusing in Git history.
  • Since the DevOps work items aren't visible from GitHub, let's reword the commit messages with a brief description of each fix. You can still link the DevOps work item in the commit's description field. See my commit in this PR for an example.

Sounds good. I'll make the updates to the commit messages.

@dpwatrous dpwatrous self-requested a review November 11, 2021 21:57
@cRui861 cRui861 force-pushed the feature/a11y-oct-21 branch from 368bfeb to 08bf7e1 Compare November 12, 2021 19:59
@dpwatrous dpwatrous dismissed their stale review November 22, 2021 18:52

Resetting feedback

dpwatrous and others added 7 commits November 22, 2021 10:54
@cRui861 cRui861 force-pushed the feature/a11y-oct-21 branch from a390b6d to 90cdd28 Compare November 22, 2021 18:57
gingi
gingi previously approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@gingi gingi left a comment

Choose a reason for hiding this comment

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

Looks good. Just 3 suggestions for localization.

src/@batch-flask/ui/buttons/refresh-btn/refresh-btn.component.ts

@Input() public type: string = "square";

@Input() public title: string = "Refresh";
Copy link
Member

@gingi gingi Nov 22, 2021

Choose a reason for hiding this comment

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

This should probably be localized by injecting the I18nService.

Copy link
Member Author

@cRui861 cRui861 Nov 30, 2021

Choose a reason for hiding this comment

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

Could you point me to an example of what you mean by this?

@cRui861 cRui861 force-pushed the feature/a11y-oct-21 branch from afe1476 to 90cdd28 Compare December 3, 2021 00:09
@cRui861 cRui861 merged commit 6ba497b into master Dec 3, 2021
@cRui861 cRui861 deleted the feature/a11y-oct-21 branch December 3, 2021 00:54
@cRui861
Copy link
Member Author

cRui861 commented Jan 31, 2022

Fixes #2415

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zfengms zfengms zfengms approved these changes

@gingi gingi gingi left review comments

@paterasMSFT paterasMSFT Awaiting requested review from paterasMSFT

@dpwatrous dpwatrous Awaiting requested review from dpwatrous dpwatrous is a code owner

@seth-barshay seth-barshay Awaiting requested review from seth-barshay

@wiboris wiboris Awaiting requested review from wiboris wiboris is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cRui861 @dpwatrous @gingi @zfengms @seth-barshay