status-subscription - Improve reliability#9849
Conversation
|
Claude's doing a pretty good job at moving only the JSX to svelte, keeping it mostly logic-free. This already seems to work |
| subscriptionButton.after( | ||
| <div className="rgh-status-subscription BtnGroup d-flex width-full"> | ||
| {tooltipped( | ||
| {label: 'Unsubscribe', direction: 'sw'}, | ||
| <Button value="unsubscribe" {...(status === 'none' && disableAttributes)}> | ||
| <BellSlashIcon /> None | ||
| </Button>, | ||
| )} | ||
| {tooltipped( | ||
| {label: 'Subscribe to all events', direction: 'sw'}, | ||
| <Button value="subscribe" {...(status === 'all' && disableAttributes)}> | ||
| <BellIcon /> All | ||
| </Button>, | ||
| )} | ||
| {tooltipped( | ||
| {label: multilineAriaLabel('Subscribe just to status changes', '(closing, reopening, merging)'), direction: 'sw'}, | ||
| <Button value="subscribe_to_custom_notifications" {...(status === 'status' && disableAttributes)}> | ||
| <IssueReopenedIcon /> Status | ||
| </Button>, | ||
| )} | ||
| </div>, |
There was a problem hiding this comment.
Broken JSX, inline spreads, pretty ugly overall. This is basically the only reason to consider svelte in status-subscription though, it's easier to have this sort of logic in svelte.
Not to mention that it was copy-pasted between the legacy and current version to simplify cleanup later on.
There was a problem hiding this comment.
the only reason
I didn't notice, but actually the component was removed and re-rendered at every click. Svelte now updates the widget instead of calling addButton recursively 🎉
This reverts commit faa7665.
|
|
||
| .rgh-clean-sidebar .thread-subscription-status .reason { | ||
| margin: 0 0 10px !important; | ||
| } |
| // TODO: Move to CSS after legacy version is dropped | ||
| function toggleSubscriptionReason(status: SubscriptionStatus): void { | ||
| // 'all' can have many reasons, but the other two don't add further information #6684 | ||
| getSubscriptionReasonElement().hidden = status !== 'all'; | ||
| } |
There was a problem hiding this comment.
This behavior was missing from the new version. Classifying this PR as bugfix
| toggleSubscriptionReason(target); | ||
| } finally { | ||
| disabled.set(false); | ||
| } |
There was a problem hiding this comment.
Maybe this whole logic can be moved in the component once the legacy version is gone (see you in 2028)
| closestElement('fieldset', event.currentTarget).disabled = true; | ||
| await updateSubscription(target, id); | ||
| void addButton(subscriptionButton); |
There was a problem hiding this comment.
Major change: instead of calling addButton recursively, the widget is now stateful and is updated instead of generated from scratch at every click.
status-subscription - Svelte componentstatus-subscription - Improve reliability
| value="subscribe_to_custom_notifications" | ||
| icon={IssueReopenedIcon} | ||
| label="Status" | ||
| tooltip="Subscribe only to closing, reopening, merging" |
There was a problem hiding this comment.
Also shortened the label and dropped the sw direction since now it automatically fits the viewport
quick-review- Improve reliability #9846PRs
Issues