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

progressbar: improve JAWS readability #1572

Merged
merged 3 commits into from
Jul 28, 2015
Merged

Conversation

sipayRT
Copy link
Contributor

@sipayRT sipayRT commented Jul 2, 2015

Closes #1511

@sipayRT sipayRT added the ready label Jul 2, 2015
@tadatuta tadatuta mentioned this pull request Jul 2, 2015
20 tasks
attrs : { style : 'width:' + (this.ctx.val || 0) + '%' }
attrs : {
style : 'width:' + val,
role : 'progressbar',
Copy link
Contributor

Choose a reason for hiding this comment

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

role and aria-valuenow attributes should be added to the root node of the block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this we should change style and aria-valuenow attributes in different places. It will be strange.
For example:

<div class="progressbar" aria-valuenow="20">
    <div class="progressbar__bar" style="width:20%"></div>
</div>

Two DOM-nodes will be changed each second while some process will be going. If this is not so terrible, I can move these attributes

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not so terrible. Let's do it.

@sipayRT sipayRT force-pushed the issues/1511@v2 branch 2 times, most recently from 7b0e039 to adfb300 Compare July 10, 2015 15:49
@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 10, 2015

@aristov all a11y attributes was moved to the root node

@@ -1,14 +1,27 @@
block('progressbar')(
def()(function() {
var val = this.ctx.val || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to create variable here.

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 24, 2015

@aristov updated

.content({
elem : 'bar',
attrs : { style : 'width:' + val + '%' }
attrs : {
style : 'width:' + val + '%'
Copy link
Contributor

Choose a reason for hiding this comment

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

Diff for no reason.

@sipayRT
Copy link
Contributor Author

sipayRT commented Jul 24, 2015

@aristov updated

@aristov
Copy link
Contributor

aristov commented Jul 28, 2015

LGTM, wait for Gemini tests passing and merge.

sipayRT added a commit that referenced this pull request Jul 28, 2015
progressbar: improve JAWS readability
@sipayRT sipayRT merged commit a5d4d46 into issues/1206-common@v2 Jul 28, 2015
@sipayRT sipayRT deleted the issues/1511@v2 branch July 28, 2015 12:47
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.

2 participants