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

fix: do not set a default id for Icon component #495

Merged
merged 1 commit into from Jul 18, 2019

Conversation

mtyaka
Copy link
Contributor

@mtyaka mtyaka commented Jul 15, 2019

Setting the id property in Icon.defaultProps to newId('Icon') makes every Icon that doesn't explicitly provide an id have an identical id property, since newId('Icon') gets eveluated at module load time. Duplicate ids are not valid HTML.

We are already invoking newId('Icon') inside the Icon component's render method if no id is provided, so we can safely set the id in defaultProps to undefined and let the default id get generated at render time.

Testing instructions

  1. Without this patch: Render two or more <Icon> components on a page. Notice that they all have the same id ('Span1').
  2. Do the same with this patch. Notice that the ids are no longer identical.

Reviewers:

@openedx-webhooks
Copy link

Thanks for the pull request, @mtyaka! I've created OSPR-3744 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jul 15, 2019
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.285% when pulling c3b1853 on open-craft:mtyaka/fix-icon-ids into 623d750 on edx:master.

@pkulkark
Copy link

@mtyaka LGTM 👍

@natabene
Copy link

@mtyaka Thank you for your contribution. @edx/fedx-admin can you give this a look when you have a chance?

Copy link
Contributor

@abutterworth abutterworth left a comment

Choose a reason for hiding this comment

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

👍LGTM

@natabene
Copy link

👍LGTM

@abutterworth Thank you. Good to merge or do you want another review from edX?

@abutterworth
Copy link
Contributor

@natabene I posted in Slack. I can merge this at the end of the day if no one brings up any concerns.

mtyaka added a commit to open-craft/paragon that referenced this pull request Jul 18, 2019
Ports fix submitted in openedx#495 to the
v3.8.0 version.
@abutterworth abutterworth merged commit 7ca4c34 into openedx:master Jul 18, 2019
@openedx-webhooks
Copy link

@mtyaka 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 6.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mtyaka
Copy link
Contributor Author

mtyaka commented Jul 18, 2019

Thanks everyone! :)

@mtyaka mtyaka deleted the mtyaka/fix-icon-ids branch July 18, 2019 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged open-source-contribution PR author is not from Axim or 2U released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants