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.

Sign up for GitHub

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

New DTI Website: Use Details/Summary For FAQ Accordion #746

Merged
merged 2 commits into from
Nov 29, 2024
Merged

New DTI Website: Use Details/Summary For FAQ Accordion #746

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9507d05
New DTI Website: Use Details/Summary For FAQ Accordion
andrew032011 Nov 28, 2024
0423af1
Remove user-select: none
andrew032011 Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 6 additions & 27 deletions new-dti-website/components/apply/ApplyFAQ.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { ReactNode, useState } from 'react';
import Image from 'next/image';
import Link from 'next/link';
import config from '../../config.json';
import interviewPrep from './data/interviewPrep.json';
Expand All @@ -9,32 +8,12 @@ type FAQAccordionProps = {
children: ReactNode;
};

const FAQAccordion = ({ header, children }: FAQAccordionProps) => {
const [isOpen, setIsOpen] = useState(false);

const handleClick = () => setIsOpen((prev) => !prev);

return (
<button
className="py-4 border-white border-b-black border-2 cursor-pointer text-left"
onClick={handleClick}
>
<div className="flex justify-between pr-4">
<p className="section-subheading">{header}</p>
<Image
src="/icons/dropdown.svg"
alt="dropdown"
width={13}
height={7}
className={isOpen ? 'rotate-180' : 'rotate-0'}
/>
</div>
<div className={`overflow-hidden ${isOpen ? 'max-h-none' : 'max-h-0'}`}>
<div className="md:py-5 xs:py-3">{children}</div>
</div>
</button>
);
};
const FAQAccordion = ({ header, children }: FAQAccordionProps) => (
<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>
Comment on lines +12 to +15
Copy link
Collaborator

Choose a reason for hiding this comment

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

yay

);

const ApplyFAQ = () => {
const sections = ['General Questions', 'Behavioral Prep', 'Technical Prep'];
Expand Down
31 changes: 31 additions & 0 deletions new-dti-website/src/app/globals.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,3 +192,34 @@
color: black;
background-color: #fefefe;
}

summary {
padding-top: 1rem;
padding-bottom: 1rem;
list-style: none;
display: flex;
align-items: center;
font-weight: bold;
justify-content: space-between;
padding-right: 1rem;
}

summary::after {
content: '';
width: 13px;
height: 7px;
background: url('/icons/dropdown.svg');
background-size: cover;
}

details[open] > summary::after {
rotate: 180deg;
}

details {
user-select: none;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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".

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, sounds good then!

}

details > *:not(summary) {
user-select: text;
}
Loading