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

Add aria-rowcount to grid #10149

Closed
wants to merge 8 commits into from
Closed

Add aria-rowcount to grid #10149

wants to merge 8 commits into from

Conversation

knoobie
Copy link
Contributor

@knoobie knoobie commented Oct 7, 2017

This pr adds basic support for aria-rowcount.

It's based on discussion from vaadin/vaadin-grid#1023 and exceeds #10046.

Please tell me what kind of Javadoc you wish for Escalator#AriaGridHelper.


This change is Reviewable

@hesara
Copy link
Contributor

hesara commented Oct 10, 2017

This seems to cause Reviewable to melt down as it sees 6 + 1984 changed files. Based on a quick look, this looks ok to me apart from the javadoc and perhaps checking if some constructor changes can break anything.

@knoobie
Copy link
Contributor Author

knoobie commented Oct 10, 2017

@hesara I can create overloading constructors so the old behaviour stays the same and add a simple if(ariaGridHelper != null) before the statements that's add or remove the rows in the AriaGridHelper.

Should I re-create the pull request (after deleting my fork and forking the current master again) to make reviewable useable again?

@hesara
Copy link
Contributor

hesara commented Oct 10, 2017

Rebasing the changes might ease the review if not too much trouble for you. I'm not sure if the constructors are an issue - perhaps @tsuoanttila has an opinion on that, although if the backwards compatible change is simple, it might be worth it to reduce the risk of breaking some add-on.

@knoobie
Copy link
Contributor Author

knoobie commented Oct 10, 2017

I will take a look at the rebasing. Thinking about the constructors.. well anything is in Escalator.java - why did I even added the AriaGridHelper in the constructor? Any InnerClass should have access to the field, so I can remove the constructor changes and call the ariaGridHelper field of the class if you don't mind!

@hesara
Copy link
Contributor

hesara commented Oct 11, 2017

Superceded by #10167.

@hesara hesara closed this Oct 11, 2017
@hesara hesara added this to the Invalid milestone Oct 11, 2017
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