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

Toggle #69

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

Toggle #69

wants to merge 5 commits into from

Conversation

geoffrey-eisenbarth
Copy link
Collaborator

@geoffrey-eisenbarth geoffrey-eisenbarth commented Oct 30, 2024

Looks like this branch accidentally contained two fixes that needed to be implemented back during the addition of [role=feed]:

  1. There was an orphaned parenthesis in sanitize.css (my mistake)
  2. Somehow the token in tokens / token of tokens thing never got implemented in 19.js. Looks like vim also stripped extra whitespace form the file.

The big feedback I'd appreciate right now is on the markup used. Please refer to the ARIA section of the documentation.

  1. Is it acceptable to ask the user to wrap label + input and input + label with e.g. .f-row.justify-content:between ?
  2. If so, should we put the onus on the user to use e.g. <label class="f-row"> if using label > input? I don't particularly like that my solution involves specifying label selectors in the aria.css file, where all the other parent selectors are just [role=thing]. It would be nice if I could move them inside the [role=switch] block somehow (or not include them).
  3. I should add, I don't particularly like that <label><input>Text</label> is justified with space between, I would prefer it if the spacing is similar to input + label, but I don't think it's possible to distinguish between <label><input>Text</label> and <label>Text<input></label>, and the latter definitely needs the spacing imo.

Also, I'd be grateful for any stylistic feedback, e.g. if the border is too thick, or if we want some other type of styling to make it look more consistent with the "missing look."

I know the button styling is broken right now, wanted to focus on getting the others right first. One question I had there was what the selector should be:

  1. button[role=switch][aria-pressed]
  2. [role=switch][aria-pressed]
  3. [aria-pressed]

I think (3) goes out the window because e.g. the collapsing navbar component uses [aria-pressed] (with .iconbutton) and shouldn't be styled like a toggle.

@dz4k
Copy link
Collaborator

dz4k commented Oct 31, 2024

Is it acceptable to ask the user to wrap label + input and input + label with e.g. .f-row.justify-content:between ?

Yes, we don't do any styling on non-nested labels anyway. Also, <form class="table"> exists.

Comment on lines +126 to +129
label[for]:has(+ [role=switch]),
label[for] + [role=switch],
[role=switch]:has(+ label[for]),
[role=switch] + label[for] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
label[for]:has(+ [role=switch]),
label[for] + [role=switch],
[role=switch]:has(+ label[for]),
[role=switch] + label[for] {
label[for]:has(+ &),
label[for] + &,
&:has(+ label[for]),
& + label[for] {

I don't particularly like that my solution involves specifying label selectors in the aria.css file, where all the other parent selectors are just [role=thing]. It would be nice if I could move them inside the [role=switch] block somehow (or not include them).

@dz4k
Copy link
Collaborator

dz4k commented Oct 31, 2024

I should add, I don't particularly like that Text is justified with space between, I would prefer it if the spacing is similar to input + label, but I don't think it's possible to distinguish between Text and Text, and the latter definitely needs the spacing imo.

I've tried before to style forms generically, it can't be done. The markup is just too powerful. I'm fine with forcing people to use table forms or layout utilities.

@dz4k
Copy link
Collaborator

dz4k commented Oct 31, 2024

Also, I'd be grateful for any stylistic feedback, e.g. if the border is too thick, or if we want some other type of styling to make it look more consistent with the "missing look."

I've made a mockup of what I'd like it to look like: https://codepen.io/deniz-a/pen/ExqLdbj

@geoffrey-eisenbarth
Copy link
Collaborator Author

Thanks! I'll try to get this dusted off in the coming days.

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