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

fix(Geosuggest): fix input id and label for #491

Merged
merged 9 commits into from
Jan 25, 2024

Conversation

joaogarin
Copy link
Contributor

@joaogarin joaogarin commented Oct 21, 2021

Description

Fixes

#490

Right now the label and id input do not work as its explained in the documentation and also how one would probably expect (the label "for" attribute would link to the input "id" :

In the readme section :

If the label and a id prop (see "Others") were supplied, a tag with the passed label text will be rendered. The element's for attribute will correctly point to the id of the element.

However what happens currently is this :

Screenshot from 2021-10-21 12-04-10

With this PR the label for and input id will match. a test was also included to insure this stays as expected in the future

Screenshot from 2021-10-21 12-00-18

Note : I do understand this could potentially break people's end to end tests which is actually how I found otu the issue myself trying to upgrade to the latest version, because this was already changed as previously the input id was the props.id directly (which is what I would expect)- This technically could be considered a breaking change, but a breaking change was already then made in #464 so I guess here is not really anything new.

Checklist

  • All tests passing
  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary
  • Commits and PR follow conventions

src/Geosuggest.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@plumdumpling plumdumpling left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request!

Just two small things.

src/Geosuggest.tsx Outdated Show resolved Hide resolved
src/input.tsx Outdated Show resolved Hide resolved
@plumdumpling plumdumpling linked an issue Aug 7, 2023 that may be closed by this pull request
@joaogarin
Copy link
Contributor Author

Will look into these today, sorry was away for vacation but happy to continue the work here

src/input.tsx Outdated Show resolved Hide resolved
test/Geosuggest_spec.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@plumdumpling plumdumpling left a comment

Choose a reason for hiding this comment

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

One last thing 🙂

src/input.tsx Outdated Show resolved Hide resolved
Co-authored-by: Leslie Lawendel <plumdumpling@users.noreply.github.com>
@joaogarin
Copy link
Contributor Author

joaogarin commented Jan 25, 2024

Any chance that this gets merged? I would love to have this fix and do an update on our end ;) thanks! if not on #511 then on the next on maybe

@yfr yfr merged commit 267b4ac into ubilabs:main Jan 25, 2024
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.

Label "for" and input "id" do not match
3 participants