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

replace .get* with .query* #49

Merged
merged 8 commits into from
Aug 25, 2014
Merged

replace .get* with .query* #49

merged 8 commits into from
Aug 25, 2014

Conversation

HenrikJoreteg
Copy link
Member

This isn't ready yet, we need to make corresponding changes to dom-bindings.

Note that I didn't keep role around making this a breaking change.

I feel like this is worth doing because according to accessibility experts the way we were using role was bad.

Also, that's what semver versioning is for, right? This will be a major version bump we publish it this way.

@HenrikJoreteg
Copy link
Member Author

Note that this is to close #18.

And addresses AmpersandJS/ampersand#21

@wraithgar
Copy link
Contributor

Is it worth keeping the variable names in both the code and examples as 'role' and 'rolename'?

@latentflip
Copy link
Contributor

I'm pretty much +1 on this, though it makes me a little uneasy deprecating role quite so quickly before we have a chance to migrate everything else. But maybe that's not a reasonable concern, I can just see some people potentially getting confused by this in the short term, but maybe that's worth it to just get past it. shrug.

We just have to make sure we major version bump everything possibly affected by this change when they upgrade to the new version.

@HenrikJoreteg
Copy link
Member Author

We could just leave it working, but we'll break it at some point. In theory this is exactly what major version bumps are for.

@HenrikJoreteg
Copy link
Member Author

I think this is ready now.

@latentflip
Copy link
Contributor

+1 from me!

@latentflip latentflip merged commit 4e858fc into master Aug 25, 2014
@bear bear deleted the renaming-get branch November 26, 2014 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants