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

Add ids to inputs and buttons #1517

Merged
merged 21 commits into from
Nov 9, 2018
Merged

Add ids to inputs and buttons #1517

merged 21 commits into from
Nov 9, 2018

Conversation

tingaloo
Copy link
Contributor

Took a stab at #1174, added ids to inputs. I assumed that there will be one instance of some inputs, correct me if i'm wrong.

Copy link
Contributor

@luisrudge luisrudge left a comment

Choose a reason for hiding this comment

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

You can have more than one lock per page, so you have to get the current lock id. There's an example here: l.id(lock) - https://github.com/auth0/lock/blob/master/src/field/email/email_pane.jsx#L21

Copy link
Contributor

@luisrudge luisrudge left a comment

Choose a reason for hiding this comment

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

Now all the ids will have be the same within a lock instance 😬

You need to mix your previous approach with this one.

For example:

const lockId = l.id(model);

...

<input id={`${lockId}-${name}`} type="hidden" value={value} name={name} />

Copy link
Contributor

@luisrudge luisrudge left a comment

Choose a reason for hiding this comment

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

Can you look the failing tests? this implementation looks nice!

@tingaloo
Copy link
Contributor Author

in chrome.jsx there is a <Header/> that contains <BackButton /> However header does not have model or lock defined so i cannot pass that to BackButton, whereas other components do so.

@luisrudge
Copy link
Contributor

@tingaloo you're just missing the snapshots! Almost there 😅

To fix snapshots, run:

yarn test:jest -- -u

@luisrudge luisrudge merged commit ebaf498 into auth0:master Nov 9, 2018
@luisrudge
Copy link
Contributor

Thanks soooooo much for this PR 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants