Open KTRosenberg opened 6 years ago
This looks good to me overall. A few things I'd comment is:
physics
and math
, I see two possible behaviours: either highlight every sketch that has physics
as well as every sketch that has math
, or highlight only sketches that have both the physics
and math
tags. In other words, either the union of those two sets or the intersection of them.
Which behaviour works best depends on what you intend the sketch tags to be: if you're looking for them to be closer to one tag per sketch, making them more like categories, then your current system (union of sets) is best. If, though, you intend to emphasize sketches' ability to have multiple tags, then it might be worth considering highlighting the intersection of those sets instead.Also, leave a comment here, or update your pull request description, when the todo list is complete, if it isn't already. :)
@Kronopath Thanks for your thoughts. The union of sets makes most sense now, so maybe one tag per sketch should be enforced after all?
Also, any thoughts on the reset functionality? I'm still not sure how it should be represented.
I'm not really sure I have much of a better idea of how to implement reset right now. Having it as a button like the tag buttongs (maybe with some slight visual differentiation) makes some sense.
Note, Please do not merge before To-Do list completed, thoughts appreciated
Changes
this.tag = "tagName"
orthis.tags = ["tagName1", ... "tagNameN"]
properties to assign a tag (or a few).To-Do
Hard-to-see but visible enough GIF demo