New DTI Website: Use Details/Summary For FAQ Accordion #746
Conversation
|
[diff-counting] Significant lines: 46. |
new-dti-website/src/app/globals.css
Outdated
| } | ||
|
|
||
| details { | ||
| user-select: none; |
There was a problem hiding this comment.
Clicking on the accordian a few times in a row leads to some of the summary text getting highlighted, which is a regression from previous behavior. So I made the details not selectable.
There was a problem hiding this comment.
hmmm...
i'm usually wary of using user-select: none; because it removes a functionality for the user (being able to select and copy an FAQ question), which might be something a user usually expects.
This StackExchange post goes more into detail about the possible drawbacks of using this particular CSS property.
I especially like when they say:
The goal in employing user-select: none should always be limited to reducing user frustration from unintended selections resulting from interaction that can easily cause them when attempting to carry out the primary task purpose of a control it is used on.
I wonder if rapidly clicking the accordion qualifies as the "primary task purpose of a control"? In my own testing when I pulled down the branch, I had to click twice pretty fast in order to have some of the summary text getting highlighted.
There was a problem hiding this comment.
I do get ur point tho andrew and it is a shame that using <details> leads to this regression, so if you feel like that adding user-select: none; is worth it in this case, I'll defer to you here.
but let's also make sure that this is an exception bc i dont think we want to make this behavior standard
There was a problem hiding this comment.
Yeah, I had a feeling it was bad practice. I reverted that part! I agree with you that rapidly clicking might be something we do when testing it, but in practice, we wouldn't do that so I wouldn't consider it to be a "primary task purpose of a control".
I'll surface this in #idol-design. |
Vannessa brought up a good point that the Application and Courses Project Showcase Accordians are for different purposes, so it makes sense to make them different. So we decided not to standardize them. |
| <details className="border-white border-b-black border-2 cursor-pointer"> | ||
| <summary className="section-subheading">{header}</summary> | ||
| <div className="md:py-5 xs:py-3">{children}</div> | ||
| </details> |
new-dti-website/src/app/globals.css
Outdated
| } | ||
|
|
||
| details { | ||
| user-select: none; |
There was a problem hiding this comment.
hmmm...
i'm usually wary of using user-select: none; because it removes a functionality for the user (being able to select and copy an FAQ question), which might be something a user usually expects.
This StackExchange post goes more into detail about the possible drawbacks of using this particular CSS property.
I especially like when they say:
The goal in employing user-select: none should always be limited to reducing user frustration from unintended selections resulting from interaction that can easily cause them when attempting to carry out the primary task purpose of a control it is used on.
I wonder if rapidly clicking the accordion qualifies as the "primary task purpose of a control"? In my own testing when I pulled down the branch, I had to click twice pretty fast in order to have some of the summary text getting highlighted.
new-dti-website/src/app/globals.css
Outdated
| } | ||
|
|
||
| details { | ||
| user-select: none; |
There was a problem hiding this comment.
I do get ur point tho andrew and it is a shame that using <details> leads to this regression, so if you feel like that adding user-select: none; is worth it in this case, I'll defer to you here.
but let's also make sure that this is an exception bc i dont think we want to make this behavior standard
Summary
For the FAQ accordion here, we should use the
<details>and<summary>tags. See this MDN article for details.Notion/Figma Link
Part of #734
Test Plan
Verify FAQ Accordion looks and behaves the same as before.
Notes
Breaking Changes