Skip to content
This repository was archived by the owner on Jun 14, 2019. It is now read-only.

More changes to test IDs #340

Merged
merged 4 commits into from
May 23, 2019
Merged

More changes to test IDs #340

merged 4 commits into from
May 23, 2019

Conversation

elvisisking
Copy link
Contributor

Here is an example: connectioncard.student-details-link. The first part, connectioncard, is the component name. The second part, student, is the name of the connection. The last part, details-link, describes the element whose test ID is defined.

@mmelko Let me know how this works and if you need any IDs changed or added.

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@gashcrumb
Copy link
Collaborator

This seems odd, I checked out the links in the drop-down of the connection card and I see:

image

where's the n. coming from? I don't think we can have . in the HTML id attribute if I remember correctly. Hopefully it didn't come from that slugify function I pointed you to 😼 if so, 😊 sorry!

@elvisisking
Copy link
Contributor Author

@gashcrumb I put the dot in to separate the component name from the rest of the ID (just seemed more readable). I can take the dots out.

@gashcrumb
Copy link
Collaborator

Ah, okay, yeah use a -- or - instead, I think dots can cause problems.

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@gashcrumb
Copy link
Collaborator

I hate to say this but I think the component name's getting compiled out, for example:

image

So every ID has a sorta random letter as a prefix since the id is produced with the minified name I guess. I don't think that part will work unfortunately, as then the IDs aren't deterministic.

Generally they don't need to be globally unique across the entire app, just to a given page. So on pages like the connection edit page you probably don't need to be too specific with the name. Just on things like the connection actions, or integration actions that get used in various pages could do with slightly more specific names, but even those include the object name in them.

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

if (prefix) {
return generateId(prefix);
}
for (let i = 0; i < values.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you do values.map(generateId).join('-') instead of this for loop etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make that change. This changes the segment separator from two dashes to one dash but that is no big deal as we talked about @gashcrumb.

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

@riccardo-forina
Copy link
Collaborator

PR Storybook available here

Copy link
Collaborator

@gashcrumb gashcrumb left a comment

Choose a reason for hiding this comment

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

👍

@pure-bot
Copy link
Contributor

pure-bot bot commented May 23, 2019

Pull request approved by @gashcrumb - applying approved label

@pure-bot pure-bot bot added the approved label May 23, 2019
@pure-bot pure-bot bot merged commit 7476cb9 into syndesisio:master May 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants