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

Add new multi_select_input tag #1518

Merged
merged 4 commits into from
Jun 9, 2021
Merged

Add new multi_select_input tag #1518

merged 4 commits into from
Jun 9, 2021

Conversation

jwoertink
Copy link
Member

@jwoertink jwoertink commented Jun 2, 2021

Purpose

Fixes #777

Description

This adds a new select tag method multi_select_input which allows you to use Arrays and pass over multiple values with a select.

# in your page
def render
  multi_select_input(operation.tags, class: "multi-select") do
    options_for_select(operation.tags, [{"Ruby", "ruby"}, {"Crystal", "crystal"}])
  end
end

Notes

There is a caveat here in that Avram Operations do not support passing in Arrays from params just yet. If you were to use this as is right now, you'd have to update your actions a bit.

post "/users" do
  SaveUser.create(params, tags: params.get_all("user:tags")) do |o, u|
      # ....
  end
end

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

Copy link
Member

@matthewmcgarvey matthewmcgarvey left a comment

Choose a reason for hiding this comment

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

Does this need a new method? What if we allowed specifying multiple on select_input and we rename the method for options that you made just to be an override?

@jwoertink
Copy link
Member Author

It doesn't necessarily need a new method. I was more thinking that since multi selects look and work a bit different, we could treat it like a separate tag. It makes it easier to grep for, and stand out in the code:

select_input(op.thing, multiple: true, size: 5, class: "select-box") do
end

multi_select_input(op.thing, size: 5, class: "select-box") do
end

I do like the idea of just doing an override on the options_for_select. I still have a few more things to work out first. I'll think more on the method naming.

@stephendolan
Copy link
Member

Yeah, for some reason I kind of like the separate methods.

Single and multi selects look and behave differently enough that it feels similar to the separation of email and text input methods we already have.

@jwoertink
Copy link
Member Author

Just pushed up #1523 which would accompany this. I'd like to get that one merged in first and then I'll come back to this one.

@matthewmcgarvey
Copy link
Member

@jwoertink you've got [DO NOT MERGE] in the title still. Is this ready for review?

@jwoertink
Copy link
Member Author

nope, not yet. After we figure out #1523 I'll come back to this. I want to still try the options_for_select with the method override.

@jwoertink jwoertink changed the title [DO NOT MERGE] Add new multi_select_input tag Add new multi_select_input tag Jun 9, 2021
@jwoertink jwoertink merged commit 86a5eba into master Jun 9, 2021
@jwoertink jwoertink deleted the issues/777 branch June 9, 2021 20:02
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.

How to use select_input with 'multiple' attribute
3 participants