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

feat(tags-input): rework to return array #469

Merged
merged 2 commits into from
Jun 9, 2023
Merged

feat(tags-input): rework to return array #469

merged 2 commits into from
Jun 9, 2023

Conversation

difernandez
Copy link
Contributor

@difernandez difernandez commented Jun 6, 2023

Motivation / Background

This Pull Request has been created because tags input has had some inconsistent behaviors in the past related to the string returned and the token that separated the tags in said string. Plus, it seemed a bit counter-intuitive that a tags/multiselect input returned a string instead of an array.

Closes #335
Closes #341
Closes #421

Detail

This Pull Request changes the tags input so it returns an array of strings instead of a token-separated string. It does so by relying on the default behavior of a regular rails select with multiple: true. This allowed me to simplify the input a lot, as now a couple of things, such as hidden input with blank value and tracking of the array value, are done under the hood.
The input now works almost identically as doing f.input :foos, as: :select, multiple: true, with the added bonus of being able to set value and display_name, and having the addable function defined for non-AR relation uses.
A disclaimer was added to inform of the return type and a gotcha present in multi selects that's important when using with Postgres' array column.

Additional information

With a Purchase model that has:

  • A foos column of Postgres array type
    • With the following to clean up the column:
    before_save do
      self.foos = foos.reject(&:blank?)
    end
  • An acts_as_taggable_on :aato_tags, from the library acts_as_taggable_on
    And an admin form with the following form:
  permit_params foos: [], aato_tag_list: []  

  form do |f|
    f.inputs do
      f.input :foos, as: :tags
      f.input :aato_tag_list, as: :tags
    end
    f.actions
  end

It looks like this:

Screen.Recording.2023-06-07.at.11.07.27.mov

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a concise description of what changed and why.
  • Tests are added or updated if you fix a bug or add a feature.
  • Documentation has been added or updated if you add a feature or modify an existing one.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature (under the "Unreleased" heading if this is not a version change).
  • My changes don't introduce any linter rule violations.

Now it is based and behaves more like a Rails select with multiple: true. Blank value and new selected options are now handled under the hood because of it
@difernandez difernandez marked this pull request as ready for review June 7, 2023 15:14
Copy link
Member

@ldlsegovia ldlsegovia left a comment

Choose a reason for hiding this comment

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

You simplified it a lot 👏🏻👏🏻👏🏻

@difernandez difernandez merged commit bf6b3a2 into master Jun 9, 2023
@difernandez difernandez deleted the fix-tags branch June 9, 2023 15:11
@timothyrobb
Copy link

Just leaving this here to confirm that this does indeed fix the issue we were experiencing with #421.
Thank you for your work! Great job simplifying it in the process 💪 🎉

@difernandez
Copy link
Contributor Author

Thanks for confirming, I'm glad it works!

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