Split country and timezone inputs to fix deprecated way of passing priority countries to country input. #1782
Conversation
|
thanks for doing the pr @nashby |
|
@nashby thanks for doing this, can you @carlosantoniodasilva please approve and merge it or write how it can be improved? Would be happy to have it, we receive to many deprecation warnings. Thanks in advance. |
There was a problem hiding this comment.
Thanks @nashby! My understanding is that this doesn't break existing country select usage, since both versions work since v2? But it fixes the deprecation warning since they're going to be removing the old argument version soon, right?
With the code as-is, I had just a minor suggestion regarding expanding the tests now with split components.
One concern that I have is that PriorityInput is probably being used by other projects, so this is technically a breaking change that would require us to release a v6 or something like that. I wonder if we should "keep" a version of PriorityInput somehow for the time being, to allow us to go with something like v5.2 instead? Perhaps we could have both CountryInput and TimeZoneInput inheriting from PriorityInput? (or even keep pointing the TZ input to that one)
We can probably leave a note to deprecate/remove it once we feel like a v6 is worth releasing, but for now I think I'd prefer to avoid that if we can, since it can affect other projects. Wdyt?
Other than that, just a changelog entry and we should be good to go. Thanks!
| with_input_for @user, :country, :country | ||
| assert_select 'select.required' | ||
| assert_select 'select[aria-required]' | ||
| end |
There was a problem hiding this comment.
We might want to add the two tests above about required to the time zone tests as well, since those aren't specific to the country one.
nashby
force-pushed
the
country-timezone-split
branch
3 times, most recently
from
0f648a9 to
1b72be7
Compare
…iority countries to country input. It still uses SimpleForm's PriorityInput for backward compatibility.
nashby
force-pushed
the
country-timezone-split
branch
from
1b72be7 to
f38c6a1
Compare
|
@carlosantoniodasilva thanks for review! Totally makes sense to keep PriorityInput so I just put it back and just used separate calls to render country and priority inputs. Does it make sense now? |
|
Looks good to me |
|
I'd love to get this deprecation warning out of my CI logs if someone has time to merge this! 🙏 |
|
@carlosantoniodasilva sorry, merged it without your approval but I think it's good to finally do it so we can fix this annoying warnings. @jaredmoody @leifcr @leifcr it's merged. Feel free to test it with master branch and report any issue! |
There was a problem hiding this comment.
@nashby sorry mate, this looks great, thanks for the fixes.
fixes #1781