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

SelectField: extend to accept more data structures as children and to fix placeholder #796

Merged
merged 43 commits into from
Jan 4, 2024

Conversation

vmilan
Copy link
Contributor

@vmilan vmilan commented Nov 8, 2023

Copy link

This pull request has been linked to Shortcut Story #352074: CN - Replace old code with SelectField component.

Copy link

github-actions bot commented Nov 8, 2023

Pull Request Test Coverage Report for Build 7396927713

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 69.205%

Totals Coverage Status
Change from base Build 7385908140: 0.0%
Covered Lines: 2609
Relevant Lines: 3446

💛 - Coveralls

Copy link

github-actions bot commented Nov 8, 2023

Visit the preview URL for this PR (updated for commit 0283cb5):

https://cartodb-fb-storybook-react-dev--pr796-feature-extend-s-xfdojm7w.web.app

(expires Wed, 10 Jan 2024 12:01:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c

@vmilan vmilan requested review from VictorVelarde and a team December 1, 2023 17:54
@vmilan vmilan changed the title Extend SelectField to accept object as value Extend SelectField to cover more use cases Dec 1, 2023
@zbigg
Copy link
Contributor

zbigg commented Dec 14, 2023

MultipleSelect seems to have excessive padding and extra colon at end (old bug)

image

@zbigg
Copy link
Contributor

zbigg commented Dec 14, 2023

(old bug) Multiple Select Field with size='medium' has same height as small, only text changes:

image

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Do we have Design check over storybook?

PD. Let's be a bit more verbose about the new 'supported use cases', at least in CHANGELOG

@vmilan vmilan changed the title Extend SelectField to cover more use cases Extend SelectField: to accept more data structures as children and to fix placeholder Dec 15, 2023
@vmilan
Copy link
Contributor Author

vmilan commented Dec 15, 2023

Code changes LGTM. Do we have Design check over storybook?

PD. Let's be a bit more verbose about the new 'supported use cases', at least in CHANGELOG

No, since they are going to review in the dedicated, I didn't want to overwhelm them with several reviews of the same thing.

Changed changelog and PR title.

@vmilan
Copy link
Contributor Author

vmilan commented Dec 15, 2023

MultipleSelect seems to have excessive padding and extra colon at end (old bug)

image

After our conversation, I decided not to export Multiple SelectField for now until we solve these errors and improve the component, hopefully next Q.

@vmilan vmilan changed the title Extend SelectField: to accept more data structures as children and to fix placeholder SelectField: extend to accept more data structures as children and to fix placeholder Jan 2, 2024
@vmilan vmilan merged commit 8d012d1 into master Jan 4, 2024
2 checks passed
@vmilan vmilan deleted the feature/extend-selectfield branch January 4, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants