Change data-id to data-shepherd-step-id on Step content elements.#282
Change data-id to data-shepherd-step-id on Step content elements.#282RobbieTheWagner merged 1 commit intomasterfrom
data-id to data-shepherd-step-id on Step content elements.#282Conversation
clashResiliance++;
BrianSipple
force-pushed
the
change-data-id-attribute-name
branch
from
46134ec to
b019124
Compare
| const content = document.createElement('div'); | ||
| const classes = this.options.classes || ''; | ||
| const element = createFromHTML(`<div class='${classes}' data-id='${this.id}' id="step-${this.options.id}-${uniqueId()}"}>`); | ||
| const element = createFromHTML(`<div class='${classes}' data-shepherd-step-id='${this.id}' id="step-${this.options.id}-${uniqueId()}"}>`); |
There was a problem hiding this comment.
I'm good w/ removing the ID attr, but since with multiple tours we can have shared ids, can we do something to make sure this value is unique?
There was a problem hiding this comment.
@chuckcarpenter I think we should have a single data-shepherd-step-id attr that does what id is currently doing.
Is that what you're suggesting?
There was a problem hiding this comment.
Then again, do we need uniqueness here?
We were discussing having a data-shepherd-active-tour attribute on the body. If we implement that, someone could always use it as the context for querying unique steps -- even if steps share ids across tours.
There was a problem hiding this comment.
@chuckcarpenter @rwwagner90 To summarize, I'm thinking we merge this PR as-is, then open up another issue for adding data-shepherd-active-tour attribute to the body and removing the extra id attribute on steps.
There was a problem hiding this comment.
I agree, we have current tour on the body, so that solves the specificity issue.
There was a problem hiding this comment.
@chuckcarpenter Where are you seeing that? I'm thinking we still need to implement it. Right now, it looks like we're just indicating the current step:
There was a problem hiding this comment.
you're right. It's in the BM frontend right now.
// TODO: add this in Shepherd.js
document.body.setAttribute('data-shepherd-active-tour', tourName);
There was a problem hiding this comment.
that said, should the tour id be part of the ID for the step?
There was a problem hiding this comment.
Follows up on the discussion [here](#282 (comment)).
Follows up on the discussion [here](#282 (comment)).
Follows up on the discussion [here](#282 (comment)).
Follows up on the discussion [here](#282 (comment)).
Follows up on the discussion [here](#282 (comment)).
3 participants
clashResiliance++;