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

Give each icon title a unique ID #45

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

nickcernis
Copy link
Contributor

Ensures each icon title has a unique ID attribute by appending the
widget's instance ID. Fixes #43 to prevent ID conflicts when
multiple widgets are active.

Props @carasmo for reporting, @dreamwhisper for suggested fix.

Ensures each icon title has a unique ID attribute by appending the
widget's instance ID. Fixes osompress#43 to prevent ID conflicts when
multiple widgets are active.

Props @carasmo for reporting, @dreamwhisper for suggested fix.
@rrennick
Copy link
Contributor

rrennick commented Dec 8, 2016

Functionally, that looks fine as long as it meets the accessibility requirement.

In other functions in the plugin $this->number is used vs WIDGET_INSTANCE_ID. For consistency, it should be used in the new function as well.

@bgardner
Copy link
Contributor

bgardner commented Dec 8, 2016

Thanks @nickcernis! We're going to push out the 2.0.1 update on this right now, just because of the cache-busting fix that @rrennick did. We can test this PR independently, etc and move it out via 2.0.2 within the next few days or so.

@nickcernis
Copy link
Contributor Author

nickcernis commented Dec 8, 2016

Thanks, @rrennick / @bgardner.

In other functions in the plugin $this->number is used vs WIDGET_INSTANCE_ID. For consistency, it should be used in the new function as well.

I was going to use $this->number directly, but it appears to be null during construction. It only takes a value when each widget is displayed, I believe. That's why I passed a template-style string of {WIDGET_INSTANCE_ID} into the HTML during __construct, to be swapped out with $this->number when each widget is instantiated and displayed. Very possible there's a smarter approach, though.

@carasmo
Copy link

carasmo commented Dec 8, 2016

Thank you @nickcernis and everyone so much!

@dreamwhisper
Copy link
Contributor

@bgardner Noting this was tested as is and working to add the ID. Will review if further changes are made.

@bgardner
Copy link
Contributor

bgardner commented Dec 8, 2016

Good to know @dreamwhisper!

@dreamwhisper
Copy link
Contributor

@bgardner Nick was correct that the number variable doesn't have a value in the constructor. I discussed this with Ron.

I think this commit is good.

@bgardner bgardner merged commit 2874435 into osompress:develop Dec 13, 2016
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.

Fails validation when mulitple instances on page
5 participants