-
Notifications
You must be signed in to change notification settings - Fork 0
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
genericizes label viewer #161
base: adg/146-scheme-names
Are you sure you want to change the base?
Conversation
Per last comment, the viewer is now called "MetaString Viewer" - mostly because we don't have a better name for it, and LabelViewer was no longer accurate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, few small things
}, | ||
acceptProps: { | ||
label: $gettext("Delete"), | ||
severity: "danger", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is defined in constants.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth noting - not defined in constants.ts for this project. Defined in arches references as a constant. Seems like we shouldn't rely on that.
}, | ||
rejectProps: { | ||
label: $gettext("Cancel"), | ||
severity: "secondary", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is defined in constants.ts
|
||
<template> | ||
<ConfirmDialog | ||
:pt="{ root: { style: { fontFamily: 'sans-serif' } } }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this repeated enough that I'm wondering if there's some base style setting we can tweak to obviate this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we could create a constant with this object value? I'm not sure there's a supported way to tweak the base style, though. cc @jacobtylerwalls
<DataTable | ||
v-model:expanded-rows="expandedRows" | ||
:value="props.metaStrings" | ||
table-style="min-width: 50rem" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks WCAG2 compliance, zooming to 200% has the table run off screen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yanked, probably copypasta. No ill effects I've noticed.
There was a request from @robgaston to genericize the label viewer so it could be used with more than just the appellative status branch. This is an attempt to do that. It pulls some of the viewer components back into the SchemeLabel section with the help of named slots. Seems to work as expected, happy to get feedback on patterns.