Skip to content
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

Create edit template for Spec and implement the placeholder for Spec Title and Desc #2230

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lucaslyl
Copy link
Contributor

@lucaslyl lucaslyl requested a review from a team February 28, 2025 13:15
@lucaslyl lucaslyl self-assigned this Feb 28, 2025
Copy link

github-actions bot commented Feb 28, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   24m 12s ⏱️ -13s
787 tests ±0  785 ✔️ ±0  2 💤 ±0  0 ±0 
792 runs  ±0  790 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit 49b0000. ± Comparison against base commit 0bfa94e.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@burieberry burieberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if separating the title and description classes is necessary, but I can take a look. This edit component does not add the sematics that I meant. As a general rule, each time there's an input field, it must have a label that's associated with it. This label could be hidden, as long as that is accessibly done (see boxel-sr-only class), and if it's clear what the input field is for for the user. The FieldContainer component normally renders as a div, but in edit format, you should give it the arg @tag='label'.

@lucaslyl
Copy link
Contributor Author

lucaslyl commented Mar 3, 2025

I don't know if separating the title and description classes is necessary, but I can take a look. This edit component does not add the sematics that I meant. As a general rule, each time there's an input field, it must have a label that's associated with it. This label could be hidden, as long as that is accessibly done (see boxel-sr-only class), and if it's clear what the input field is for for the user. The FieldContainer component normally renders as a div, but in edit format, you should give it the arg @tag='label'.

@burieberry Thank you for reviewing this. I separated the title and description classes to meet the design requirements for the in-place edit input fields with placeholders. The title field needed special styling with a larger font.
I did add IDs to the BoxelInput components so that labels could be properly associated with them for accessibility.

I tried using <FieldContainer @Label='Title' @tag="label" ...> to wrap the fields, but there wasn't a good way to hide the label. The only option was using deep selectors to make the label hidden with the boxel-sr-only class. Perhaps we could improve the FieldContainer component to better address this accessibility issue (eg: @Args showLabel)?

For now, I chose to use normal labels with the boxel-sr-only class, similar to what's done in the chat-input-container. Please let me know if you think this is a good approach or if you have other suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants