Skip to content

Framework: Remove component-uid dependency #17560

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
Closed
wants to merge 3 commits into from
Closed

Framework: Remove component-uid dependency #17560

wants to merge 3 commits into from

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Aug 28, 2017

This PR updates the Popover component to use lodash's uniqueId instead of component-uid. uniqueId does work a little differently (it uses an incremental ID instead of a randomly generated one), but that's just fine for this use case, as long as it is still unique.

This is also the last usage of component-uid, so we can remove it altogether - this is also suggested in this PR.

To test:

  • Checkout this branch.
  • Go to a page with more than one popover and verify that they works well.
  • Verify Calypso boots, loads and works properly and tests still pass.

@tyxla tyxla added Components Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Aug 28, 2017
@tyxla tyxla self-assigned this Aug 28, 2017
@tyxla tyxla requested review from ockham and gwwar August 28, 2017 11:41
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=remove/component-uid

@matticbot matticbot added the [Size] M Medium sized issue label Aug 28, 2017
gwwar
gwwar approved these changes Aug 30, 2017
View reviewed changes
Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Tests well for me. Thanks @tyxla!

@gwwar gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 30, 2017
@gwwar gwwar mentioned this pull request Aug 30, 2017
Merged
aduth
aduth requested changes Aug 30, 2017
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Would like to merge #17356 instead to avoid the rebase.

@aduth
Copy link
Member

aduth commented Aug 30, 2017

Superseded by #17356

@aduth aduth closed this Aug 30, 2017
@tyxla tyxla deleted the remove/component-uid branch August 30, 2017 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth requested changes

@ockham ockham Awaiting requested review from ockham

+1 more reviewer

@gwwar gwwar gwwar approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@tyxla tyxla

Labels
Components Framework [Size] M Medium sized issue [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tyxla @matticbot @aduth @gwwar