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.

Sign up for GitHub

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

Support Range :values in catalogue #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −6
Open

Support Range :values in catalogue #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/surface/catalogue/components/component_api.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ defmodule Surface.Catalogue.Components.ComponentAPI do
"—"
end

defp format_values(%Range{} = range) do
raw(["<code>", inspect(range), "</code>"])
end

defp format_values(values) do
values
|> Enum.map(fn value -> raw(["<code>", format_value(value), "</code>"]) end)
Expand Down
9 changes: 3 additions & 6 deletions lib/surface/catalogue/components/prop_input.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,12 @@ defmodule Surface.Catalogue.Components.PropInput do
defp css_value_to_string(nil), do: nil
defp css_value_to_string(value), do: Enum.join(value, " ")

defp get_choices(prop) do
values =
prop.opts
|> Keyword.get(:values, [])
|> Enum.map(&{&1, &1})
defp get_choices(%{opts: opts}) do
values = for x <- Keyword.get(opts, :values, []), do: {x, x}

cond do
values == [] -> []
prop.opts[:required] -> values
opts[:required] -> values
true -> [{"nil", "__NIL__"} | values]
end
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's better to just return [] if :values is a range. This way we'll have a text input instead of a select. It would be weird to have a select with all values of a large range, e.g. 1..1000.

Another approach would be to check the number of values, something like using a select if the number of items Is <= 10 and leaving as a text input if it's > 10. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a good point @msaraiva. I had considered it but did not have a good solution, this seems like a reasonable approach. I would possibly make it 20-25 rather than 10 though. Sound good?

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with that. Just make sure you try it out to see if it doesn't affect usability badly. I mean, I'd rather type 16 myself than look for a 16 item in a 20-sized list :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea i'll see what the UX looks like. my thought was that 10 might be too small. Looking for that sweet spot 🍯

Copy link
Member

@msaraiva msaraiva Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, we could have an editable select similar to https://vue-select.org/ or use a <input> + <datalist> instead of a <select>.

end
Expand Down