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

Render a dropdown list when an inclusion validator is used #925

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

shalvah
Copy link
Contributor

@shalvah shalvah commented Dec 3, 2023

This allows you to write:

attribute :limit, :integer
attribute :type, :string

validates :limit, inclusion: [4, 6, 8]
validates_inclusion_of :type, in: ['new', 'old']

And get:

image

instead of:

image

Reduces the chance of making mistakes, saves work, serves as documentation for unfamiliar users (tells them what options are available).

@nvasilevski
Copy link
Contributor

hey @shalvah, thanks for the PR! Could you have a look on the CI failures?

I think this feature makes sense and I don't have concerns with it. It uses mostly public APIs apart from InclusionValidator but I think we can bear with it to have this feature in. I'll double-check with my teammates

@shalvah
Copy link
Contributor Author

shalvah commented Dec 17, 2023

FYI, tests are currently failing because of a ChromeDriver change which broke Capybara (teamcapybara/capybara#2725). They've fixed it, but not released yet, so I guess we have to wait a bit. This affects other PRs in this repo too.

@etiennebarrie
Copy link
Member

The Selenium issues were resolved in #936.

@shalvah
Copy link
Contributor Author

shalvah commented Dec 18, 2023

Nice, thank you! Updated the branch, should pass now.

@nvasilevski
Copy link
Contributor

Thanks @shalvah ! CI is green, but may I ask you to add a test for the feature? And this will be good to be merged

@shalvah
Copy link
Contributor Author

shalvah commented Dec 21, 2023

Test added ✅ .

Also, could someone please take a look at #924 ? I know it's a more involved design change, but it would be really helpful for many use cases, as explained in the description.

Copy link
Contributor

@nvasilevski nvasilevski 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!

(there is one failing CI job which is related to Rails edge not supporting ruby 3 anymore, but unrelated to the PR)

@nvasilevski nvasilevski merged commit dfd3ddd into Shopify:main Jan 9, 2024
16 of 17 checks passed
@shalvah
Copy link
Contributor Author

shalvah commented Jan 10, 2024

Thanks!

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.

4 participants