Add tags to patches #69
Conversation
A Tag is an arbitrary label for a patch in the Commitfest UI. Other than helping users identify patches of interest, it has no other semantic meaning to the CF application. Tags are created using the administrator interface. They consist of a unique name and a background color. The color should be sent from compliant browsers in #rrggbb format, which is stored without backend validation; to avoid CSS injection, any non-conforming values are replaced with black during templating.
Add front-end soft validation to the TagAdmin form. This uses WCAG 2.2 guidelines to warn the administrator if a color choice is going to be low-contrast when compared to our text/background color of white. (The warning may be ignored.)
| @@ -531,6 +533,7 @@ def patchlist(request, cf, personalized=False): | |||
| ) | |||
|
|
|||
|
|
|||
| @transaction.atomic # tie the patchlist() query to Tag.objects.all() | |||
There was a problem hiding this comment.
I'd be inclined to define Tag as an append/update-only table - soft deletes only. Think PostgreSQL enums in recommended usage (but with a soft delete capability). No real point enforcing a transaction to read its contents in that case. Even if we got an overly fresh read the attributes of Tag that would be valid data.
There was a problem hiding this comment.
Until tag usage settles, I think it should be easy for CFMs to add and remove them. (Easily worth a transaction wrap IMO, and the more I look at the complexity of patchlist() the more I think that perhaps it should have been wrapped already...)
Is there some hidden cost to @transaction.atomic that's not in the documentation, that I should be aware of?
| class Tag(models.Model): | ||
| """Represents a tag/label on a patch.""" | ||
|
|
||
| name = models.CharField(max_length=50, unique=True) |
There was a problem hiding this comment.
I suppose it might be considered pre-mature but I'd have a separate "label" attribute from the "Name" of the tag (and/or a description) - label being displayed and optimized for length, name and description being documentation. I'd also just add a "tag_category" right up front. I'd tie the colors to the categories and then distinguish within them by name. Given the expanse of the "Product Module" category maybe we should consider a two-color scheme: background for the category and border (and label) to denote the specific element within the category. I'd strongly advise we decide on the initial set of tags before getting to far along in design and development though (lets figure out where to collaborate on that). For categories, at minimum Front-End and Back-End come to mind, probably Protocol. That doesn't include stuff like "Good First Issue" which might fall into a "Meta" category. I'm envisioning stuff like a "CirrusCI" category that flags for stuff like "wants clobber cache enabled" (this is less well formed in my mind but stemmed from the recent discussion on Discord).
There was a problem hiding this comment.
Right now it's difficult to get people to agree on what Tags should even be used for, so I'd like to keep it super simple at first. We can add features as requested.
| class Patch(models.Model, DiffableModel): | ||
| name = models.CharField( | ||
| max_length=500, blank=False, null=False, verbose_name="Description" | ||
| ) | ||
| topic = models.ForeignKey(Topic, blank=False, null=False, on_delete=models.CASCADE) | ||
| tags = models.ManyToManyField(Tag, related_name="patches", blank=True) |
There was a problem hiding this comment.
Maybe also pre-mature, and likely reasonably added later, but I envisioned an ability for annotations to be added to the tag-patch mapping. Maybe the commit message and email threads are enough places to write stuff but, especially for something like 'Good First Issue', some commentary as to why would be expected to exist somewhere, and a tag annotation seems like a good place.
There was a problem hiding this comment.
I'm also thinking we'll use the mailing list for primary discussion while this shakes out, but I think this would be a good thing to add eventually.
|
For the CSS I've a couple of thoughts. Can we use "LCH" as the color specifier instead of RGB? Python has cssutils to dynamically build stylesheets. Not sure we can fully delegate value validation but keeping the legibility check in place is nice which naturally checks the inputs. I'd be more inclined to add data-* attributes to the html markup and build a dynamic stylesheet served from an assets URL. Solving cache busting/invalidation (version code in the virtual file name) should be straight-forward - tags aren't going to be frequently changing. I'm considering whether they should just be compiled in; do away with the creation UI altogether. Point users to an external color picker and just tell them where to copy the value. Need to do more research here though - just some thoughts for now. |
I like the technical details of LCH, but does it give users something? It's hard to beat "click the button, pick a color, move on" for the price of an |
A Tag is an arbitrary label for a patch in the Commitfest UI. Other than helping users identify patches of interest, it has no other semantic meaning to the CF application. This addresses #11 and #67.
Tags are created using the administrator interface. They consist of a unique name and a background color. The color should be sent from compliant browsers in
#rrggbbformat, which is stored without backend validation; to avoid later CSS injection, any non-conforming values are replaced with black during rendering.The second patch helps admins pick colors that aren't completely eye-gouging, by putting up an (ignorable) warning if we don't meet baseline WCAG recommendations on text contrast.
Notes
patchlistquery, I instead grab the tag IDs and look them up in a map at render time. I think this requires@transaction.atomicto tie the map and patches together, which I've added across the entire view for simplicity.data-*attribute, but maybe someone knows of a way to do that? That would get rid of the CSS injection problem altogether.TODOs
Screenshots
Admin interface:
User interface: