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

DotTip: Rename id attribute to tipId #10317

Merged
merged 2 commits into from
Oct 27, 2018
Merged

DotTip: Rename id attribute to tipId #10317

merged 2 commits into from
Oct 27, 2018

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Oct 3, 2018

Resolves #10305

Description

This PR renames id prop on <DotTip /> to tipId to avoid using common DOM attributes as prop names.

It also removes the eslint exception in the selector, thus reverting it back to its original rule.

How has this been tested?

Manually tested by viewing NUX tips in a new post. Execute localStorage.setItem('WP_DATA_USER_1', '') in the console to remove user data from locale storage if you've already dismissed tips before

I also ran both unit and e2e tests.

npm run lint against changes. Also ran against original props, e.g., <DotTip id="core/editor.publish"> and received the following error: error Do not use string literals for IDs; use withInstanceId instead no-restricted-syntax

Screenshots

screen shot 2018-10-03 at 4 50 01 pm

Types of changes

This PR renames the id prop to tipId, and correcting related side effects in the tests/snapshots.
We're also updating the documentation to reflect this change, and removing the exception in the eslint selector.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ramonjd
Copy link
Member Author

ramonjd commented Oct 5, 2018

Note to reviewer: different e2e tests are failing each time I push. I can't replicate anything consistent locally.

@ajitbohra ajitbohra added [Feature] NUX Anything that impacts the new user experience [Type] Enhancement A suggestion for improvement. labels Oct 19, 2018
Copy link
Member

@ajitbohra ajitbohra left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jasmussen jasmussen added this to the 4.2 milestone Oct 23, 2018
@@ -58,19 +58,19 @@ export function DotTip( {
}

export default compose(
withSelect( ( select, { id } ) => {
withSelect( ( select, { tipId } ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we support id as well in conjunction with a deprecated call?

…void using common DOM attributes as prop names.

Updates prop names where we're using the component, including in README examples and documentation.
Updates related unit tests and snapshot.
Removes eslint exception.
@youknowriad
Copy link
Contributor

Thanks for the PR @ramonjd I pushed a change add the deprecation, I hope you don't mind :)

@ramonjd
Copy link
Member Author

ramonjd commented Oct 27, 2018

I pushed a change add the deprecation

No worries @youknowriad ! I had a change ready to commit, but wasn't sure about what to write in the deprecation text or which version was going out next, so I'm glad you got in first :)

I had something like:

// Prop `id` in <DotTip /> is deprecated.
function getIdProp( id ) {
	if ( id ) {
		deprecated( 'Prop `id` in <DotTip />', {
			alternative: 'tipId',
			hint: 'Don\'t use an `id` prop for components that do not require an instanceId',
		} );
	}
	return id;
}

...
	withSelect( ( select, { tipId, id } ) => {
		const dotTipId = tipId || getIdProp( id );
...

Thanks!!

@youknowriad
Copy link
Contributor

Oh sorry, I should have waited, both work though :). Let's wait for the test and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] NUX Anything that impacts the new user experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants