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.

Sign up for GitHub

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

Jump to bottom

fix: support aria-controls with multiple ids in Toogler #11167

Merged
merged 3 commits into from
Apr 13, 2018
Merged

fix: support aria-controls with multiple ids in Toogler #11167

merged 3 commits into from
Apr 13, 2018

Conversation

MoarCoding
Copy link

@MoarCoding MoarCoding commented Apr 12, 2018 •
edited by ncoden
Loading

Closes #11166

-- edited by @ncoden

@MoarCoding MoarCoding mentioned this pull request Apr 12, 2018
Closed
DanielRuf
DanielRuf reviewed Apr 12, 2018
View reviewed changes
js/foundation.toggler.js Outdated
@@ -65,6 +65,21 @@ class Toggler extends Plugin {
'aria-controls': id,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still the old logic left, var id and others are set multiple times which can cause issues.
It might make more sense to create new variables in the new code block or extend the old logic with the new one.

Copy link
Author

Choose a reason for hiding this comment

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

Quite right! Missed that one. I'll have another look to make sure that I haven't missed anything outside of the changed code. PR coming soon.

@MoarCoding
Copy link
Author

I removed the old code, but now I get a failed check. I can't see what I did wrong so could you take a look?

@DanielRuf
Copy link
Contributor

The Travis test? Well, it seems there was a timeout accessing the Browserstack API in general. I'll try to rerun the test.

@MoarCoding
Copy link
Author

Thanks!

@DanielRuf
Copy link
Contributor

The Browserstack test successfully ran now. You can also do it by clicking on the retry button.

bildschirmfoto 2018-04-12 um 15 45 18

https://travis-ci.org/zurb/foundation-sites/builds/365598109 (you get there by clicking on the green checkmark / red cross next to your commit).

@MoarCoding
Copy link
Author

Thanks. Good to know.

@MoarCoding MoarCoding closed this Apr 12, 2018
@DanielRuf
Copy link
Contributor

DanielRuf commented Apr 12, 2018 •
edited
Loading

Hm, why did you close the PR?
Was it by accident?

@MoarCoding MoarCoding reopened this Apr 12, 2018
@MoarCoding
Copy link
Author

More through inexperience. =) I've only just begun using git and this is my first contribution to open source. I'll study https://foundation.zurb.com/get-involved/code-contributor-guide.html a bit more.

@DanielRuf
Copy link
Contributor

Ah, your first contribution here. Congrats 🎉
We can also push changes to your branch and can give you feedback through reviews.

Great that you spotted the issue.

So far everything is ok and tests are stilk ok. I'll try to test your changes later.

@ncoden ncoden changed the title fixed bug where aria-controls is not updated with multiple ids fix: support aria-controls with multiple ids in Toogler Apr 12, 2018
@ncoden
Copy link
Contributor

ncoden commented Apr 12, 2018 •
edited
Loading

Hi @MoarCoding. Thanks for your issue & PR, that's a nice first contribution 🎉!

I have a few comments to make about it. I hope it will help you to improve yourself in Git and working on Open Source projects.

  • Write your commits names and Pull Request title in the "imperative" form.
    It must describe "what this commit/PR does", like if the sentence was starting by now the software should.... I renamed your PR to ...support aria-controls with multiple ids in Toogler
  • Use prefixes for commits names and Pull Request title
    like feat (new feature), fix (you repair something), docs (documentation), refactor (you change the code without introducing changes), style (you simply change code formatting), tests (units or visual tests) or chore (for boring day-to-day tasks not touching the actual code). I would recommend you to use them for Foundation, but also your others projects. You can take a look at the AngularJs Git Commit Message Convention also.
  • Describe everything you did and why in your commits and Pull Requests body.
    Even if you talked about it in the PR you're resolving. It's always helpful later. Explains what you did if the code is not clear enough (but it should be) and for which reasons. Give references (in this case the W3C Reference about aria-controls would be useful. You should always reference the issue you are closing (like Closes #11166).
  • Configure Git so it uses your GitHub email address.
    See https://help.github.com/articles/setting-your-commit-email-address-in-git/. This way we can know you are the author of these commits. You can also sign them if you like.
  • Always open a pull request from a dedicated branch.
    Like fix/toggler-support-aria-controls-multiple-ids-11166), so you develop branch stay clean and you can provide multiple Pull Requests for various issues.

And finally, here are some helpful ressources for Git: 📚

ncoden
ncoden suggested changes Apr 12, 2018
View reviewed changes
js/foundation.toggler.js Outdated

//make sure not to duplicate id in aria-controls
$this.attr({
'aria-controls': `${$this.attr('aria-controls') ? $this.attr('aria-controls').replace(id, '') : ''} ${id}`.trim(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This would actually remove test1 inside id="test15 test25" and change it to id="5 test25 test1". You should use RegExp instead with the boundary special character \b.

js/foundation.toggler.js Outdated
'aria-expanded': this.$element.is(':hidden') ? false : true
var id = this.$element[0].id,
$triggers = $(`[data-open~="${id}"], [data-close~="${id}"], [data-toggle~="${id}"]`),
that = this;
Copy link
Contributor

@ncoden ncoden Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Foundation uses Babel, so you can use all ES2015 features like arrow functions. See https://github.com/lukehoban/es6features

I think this part could be cleaned a bit. I'll take care of this.

@ncoden
fix: improve id detection/replacment for Toggler aria-controls attr…
6437acf 
…ibute

Changes:
* Only add the id if it does not exist in `aria-contolers` with better RegExp detection
* Use ES6 features to simplify code
* Add comments
ncoden
ncoden approved these changes Apr 12, 2018
View reviewed changes
Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

@MoarCoding I took care of the requested changes. Please let me know what you think about it ;)

@ncoden ncoden requested a review from DanielRuf April 12, 2018 22:18
DanielRuf
DanielRuf approved these changes Apr 13, 2018
View reviewed changes
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

works very good, did not see any issue

@MoarCoding
Copy link
Author

MoarCoding commented Apr 13, 2018 •
edited
Loading

Thanks, @ncoden! I'll make sure to tick those boxes when submitting my next PR.

@DanielRuf DanielRuf mentioned this pull request Apr 13, 2018
Merged
@ncoden ncoden merged commit d312a5b into foundation:develop Apr 13, 2018
@ncoden ncoden added this to the 6.5.0 milestone Apr 24, 2018
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
@StelisKnowit @ncoden
Use pull request foundation#11167 from MoarCoding/develop for v6.5.0
ce513bc 
2bfd3e0 fixed bug where aria-controls is not updated with multiple ids
3e6e35c Removed old code
6437acf fix: improve id detection/replacment for Toggler `aria-controls` attribute

Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
This was referenced Jun 16, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRuf DanielRuf DanielRuf approved these changes

@ncoden ncoden ncoden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MoarCoding @DanielRuf @ncoden @StelisKnowit