-
Notifications
You must be signed in to change notification settings - Fork 12
Cs 10042 make field configuration playground editable without server writes for unauthorized user #3848
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
base: main
Are you sure you want to change the base?
Conversation
Preview deployments |
Host Test Results 1 files ± 0 1 suites ±0 1h 37m 10s ⏱️ - 41m 13s Results for commit 81e5c18. ± Comparison against base commit 75067cd. This pull request removes 1 and adds 233 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| <button | ||
| type='button' | ||
| class='copy-button' | ||
| title='Copy code' | ||
| {{on 'click' this.copyCode}} | ||
| > | ||
| {{#if this.isCopied}} | ||
| <CopyCheckIcon width='14' height='14' /> | ||
| <span>Copied</span> | ||
| {{else}} | ||
| <CopyIcon width='14' height='14' /> | ||
| <span>Copy</span> | ||
| {{/if}} | ||
| </button> |
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.
Use the boxel-ui/copy-button here: https://boxel-ui.stack.cards/?s=Components&ss=%3CCopyButton%3E. You can send an arg for what shows up in the tooltip too.
| Element: HTMLDivElement; | ||
| } | ||
|
|
||
| export default class CodeSnippet extends GlimmerComponent<CodeSnippetSignature> { |
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.
Check out the CSSField in card-api and CSSValueField in base realm. I think the component you want here is similar to those.
| background-color: var(--muted, #f1f5f9); | ||
| border: 1px solid var(--border, #e0e0e0); |
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.
So it doesn't actually help to have these fallback values hardcoded like this all over the css because they may need to be different for dark mode for example. It's better to specify it in the beginning like: --_code-muted: var(--muted, #f1f5f9) and then use var(--_code-muted) in places where you need it. This is for colors only
| font-family: var( | ||
| --font-mono, | ||
| 'Monaco', | ||
| 'Menlo', | ||
| 'Ubuntu Mono', | ||
| 'Consolas', | ||
| monospace | ||
| ); |
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 should be: var(--font-mono, var(--boxel-monospace-font-family))
| border-radius: var(--radius, 0.375rem) var(--radius, 0.375rem) 0 0; | ||
| } | ||
| .code-snippet-label { | ||
| font-family: var(--font-sans, system-ui, 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.
you shouldn't need to specify the sans-serif, I think it's default
| } | ||
| .code-snippet-label { | ||
| font-family: var(--font-sans, system-ui, sans-serif); | ||
| font-size: 0.75rem; |
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 var(--boxel-caption-font-size)
| font-weight: 500; | ||
| color: var(--muted-foreground, #64748b); | ||
| text-transform: uppercase; | ||
| letter-spacing: 0.05em; |
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.
--boxel-lsp-xxs
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.
Thanks for your advise! I've updated the styling based on your feedback and aligned it with the new variable guidelines in variables.css
| > | ||
| {{#if field.component}} | ||
| <field.component @format='edit' /> | ||
| <field.component @format='edit' @canEdit={{true}} /> |
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.
@lukemelia this is where lucas implemented the usage of the box component. Here the field showcase fields need to be be able to be interacted with ( Not disabled ) so we have to flag the UI to render the component as tho its editable.
Here the motive of rendering with a box component here is lucas created a generic component field-spec-template bcos he didn't want to make the components on the spec shareable and he didn't want to repeat hardcoded templates with @fields instantitions -- otherwise, he wud have to do it with all the fields. FieldRenderer was a previous component introduced by me to support rendering all types of fields inside of a table form (this component does some internal implementation to prevent flickering during edits). Looks like he used this component and hence has access to the boxComponent itself.
In my opinon, I don't see usage of box component too bad but even if we just stuck to a pattern whereby we lock ourselves into @fields, I think we wud still need to string thru an arg like @canedit to the box component to get the behaviour we want. In addition, lucas didnt have to fuse the field-renderer -- it was just a convenience for him
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 think in addition,
canEdit is a funny name for the api layer we r changing.
I was thinking an argument like displayAsEditable might be clearer. and it separates from the understanding of canEdit
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.
@tintinthong I think I'm going to need a walkthrough on this. These spec field showcases should function as examples for how to use the fields. More verbose isolated & edit templates with standard field delegation would be better than a clever shared template that uses something most card authors should likely not.
linear: https://linear.app/cardstack/issue/CS-10042/make-field-configuration-playground-editable-without-server-writes-for
Changes
Add canEdit prop override to BoxComponent for field editability control
Implement CodeSnippet component on field-spec isolated and edit templates
Add @disabled support to form inputs across catalog-realm fields:
Fields now respect canEdit prop to disable editing when write permissions are not available
Users without write permissions can still view and interact with fields in read-only mode
Result:
Screen.Recording.2026-01-15.at.9.54.56.PM.mov