search

osompress / simple-social-icons

Plugin: Simple Social Icons
62 stars 33 forks source link

Give each icon title a unique ID #45

Closed nickcernis closed 7 years ago

nickcernis commented 7 years ago

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.

rrennick commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Thank you @nickcernis and everyone so much!

dreamwhisper commented 7 years ago

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

bgardner commented 7 years ago

Good to know @dreamwhisper!

dreamwhisper commented 7 years ago

@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.