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

select/multi_select: support for adding new tags if some of the tags don't exist (fix for issue 51) #187

Closed
wants to merge 1 commit into from

Conversation

de-novikov
Copy link

The pull request fixes the issue described in issue 51:
#51

@de-novikov de-novikov changed the title select/multi_select: support for adding new tags if some of the tags don't exist (fix for Issue 51) select/multi_select: support for adding new tags if some of the tags don't exist (fix for issue 51) Sep 9, 2020
@jamalex
Copy link
Owner

jamalex commented Nov 22, 2020

Fantastic, thanks for your work on this!

I would suggest a couple of changes:

  • It's a bit risky to overwrite the schema entirely, based on the current local value, since many people are running with auto-refreshing turned off, so it may be stale. To be a bit gentler, perhaps consider something like c.set("schema.{}.options".format(prop["id"]), new_options_list) (with new_options_list being the old list with the new item appended).
  • It could be valuable (and keep things cleaner) to move a lot of this logic to a separate method, on Collection, for adding a property to a collection. I might suggest Collection.add_select_option(prop, value, color=None) and Collection.has_select_option(prop, value) for checking existence.

Longer-term, I'd love to have an object-oriented way of dealing with a Schema, which would be a good place to handle this sort of thing, but great to cover this common use case in the meantime.

@jamalex
Copy link
Owner

jamalex commented Jan 25, 2021

This is now possible via #252 -- if you'd like to add anything on top of what is there, please feel free -- thanks!

@jamalex jamalex closed this Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants