Conversation
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 2, 2026 20:07
804105e to
d9fc688
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 2, 2026 20:27
d9fc688 to
f25c772
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
2 times, most recently
from
September 2, 2026 21:11
249e325 to
c4d086d
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 4, 2026 16:58
c4d086d to
bcfa5e4
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
2 times, most recently
from
September 9, 2026 00:48
f0ae2ba to
f644fa9
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 9, 2026 22:31
f644fa9 to
4980f35
Compare
Contributor
|
I feel like Tooltips are a bit difficult to discover and maybe not the correct component to display help text. I wonder if Claude Design could help come up with a more intuitive layout that also allows us to display dynamic help text in the dialog? I think the "Propagate to roles" toggle should probably go above all the other toggles since it affects their behavior? Maybe it should be above the "Optional Constraints" section as propagating to roles is not necessarily a constraint, but a modifier of the constraints? |
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 10, 2026 21:14
4980f35 to
dac1e1f
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
2 times, most recently
from
September 10, 2026 23:46
c104131 to
7708b26
Compare
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 11, 2026 00:00
7708b26 to
c111574
Compare
`propagate_to_roles` gave the tag form a seventh toggle whose effect on the other six was left to the reader to work out. Only the toggle itself carried a tooltip; the six constraints were bare labels. Each now carries help text. On the form it describes both cases, so an admin choosing whether to propagate can see what it means for the constraint in front of them. On the tag page it describes only the behaviour in force, read off that tag's own propagation setting and value. The wording turns on a distinction worth stating explicitly, since it is easy to get backwards. Every constraint governs access *to the tagged group* by either route -- a user's own, and the association of a role that carries its members in -- whatever the gate says. What propagation adds is that the same constraint governs membership *of the role itself*. So "without propagation" is not "no constraint": a role's access to the group is still bounded, and what is unbounded is who is in the role. The owner side then crosses axes, because an owner limit reaches roles that *own* the group and lands on those roles' *membership*. The self-add pair gets no "without propagation" paragraph. #617 rejects that combination on every write, so describing it would document a configuration the backend refuses; the copy says why the setting is required instead. Labels move into one `CONSTRAINT_LABELS` map shared by both views, which until now named the same constraint two different ways -- `Member Time Limit` against `Member time limit`, `Owners may not add selves as owners?` against `Disallow owners adding selves as owners`. The self-add wording also changes to "adding oneself": `put_group_members` authorizes against the target, so adding yourself to a role requires owning the *role*, and the restriction reaches people who own neither the role nor the tagged group. Copy lives in a pure `constraintHelp.ts` as structured paragraphs rather than prose, because MUI's `title` takes a ReactNode and would render markdown literally -- and because this repo's style engine cannot load under vitest, so anything rendered is untestable. Two changes to what the tag page lists follow from dropping the "not set" variants: - Rows appear only for constraints in force. The form writes all four boolean keys on every save, so a typical tag stored several switched off and listed them as rows reading "No". `Total:` counts what is listed. - A tag that constrains nothing says so, rather than showing an empty-list dash, and the propagation note is suppressed there -- "these constraints do apply to roles" directly above "does not apply any constraints" contradicts itself. Also routes the Value column's time limits through `timeLimitLabel`. It divided by 86400 unrounded, so a one-hour limit read `0.041666666666666664 days`; the tooltip renders the same number, and the two would have disagreed on one row.
…eyboard A table whose only row explains that it has no rows is worse than no table: the column headers, the "Total: 0" and the propagation note all describe constraints that do not exist. A tag that constrains nothing now gets the sentence alone. The tooltips were also mouse-only, which left the copy unreachable for the people most likely to need it read to them. Two causes, both now fixed for all seven of the form's tooltips and all six of the tag page's: - The trigger was not focusable, so there was no way to open a tooltip without a pointer. `tabIndex` on the trigger makes MUI open it on focus. - MUI treats a tooltip title as the child's accessible *name* by default. For a paragraph of prose that means a screen reader announces the whole explanation where the label belongs, and never the label itself -- verified against the accessibility tree, where the propagation label's name was its entire tooltip. `describeChild` makes it a description instead, so the label is the name and the help text is `aria-describedby`. Both live in one `ConstraintHelpTooltip` that the two views share, rather than being repeated at thirteen call sites. The propagation toggle's own tooltip predates this work and keeps its string copy, but it had both problems and gets the same treatment; a string title additionally earns it a native `title` attribute from `describeChild`, so its closed state is described too. Confirmed end to end: a real Tab sets `:focus-visible`, which is what MUI gates on, the tooltip opens, and `aria-describedby` resolves to the full text. A programmatic `.focus()` does not, so this is only observable with genuine key events.
Three comments cited `#617` for the rule that a self-add restriction requires propagation. A PR number tells a later reader nothing; point at `PROPAGATION_REQUIRED_CONSTRAINT_KEYS`, which is what enforces it and what they can go read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
barborico
force-pushed
the
brynna/tag_constraint_tooltips
branch
from
September 11, 2026 00:15
c111574 to
ebb651b
Compare
barborico
marked this pull request as draft
September 18, 2026 19:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Urgency: none at all
Expected review effort: LOW — copy and one table filter
Motivation
propagate_to_rolesgave the tag form a seventh toggle whose effect on the other six was left to the reader to work out. Only the toggle itself carried a tooltip.Review feedback on #617:
What's changing
Each constraint now carries help text. The form describes both cases, so an admin choosing whether to propagate sees what it means for the constraint in front of them. The tag page describes only the behaviour in force, read off that tag's own propagation setting and value.
Two consequences the copy expresses: an owner limit reaches roles that own the group and lands on those roles' membership, since membership of the role is what confers the ownership; and the self-add pair describes only the propagating case, because #617 rejects the other one and describing it would document a configuration the backend refuses.
Edit view
Read view
Impact beyond the copy
Member Time LimitvsMember time limit, and so on), which is its own source of confusion. The self-add labels also become "Disallow adding oneself as…": adding yourself to a role requires owning the role, so the restriction reaches people who own neither it nor the tagged group, and naming an owner of the tagged group was simply wrong.0.041666666666666664 days.🤖 Generated with Claude Code