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

added custom focus states #784

Merged
merged 5 commits into from
Dec 8, 2024
Merged

added custom focus states #784

merged 5 commits into from
Dec 8, 2024

Conversation

Bookie0
Copy link
Collaborator

@Bookie0 Bookie0 commented Dec 7, 2024

fixes this

X Before After
Profile card Screenshot 2024-12-07 at 5 05 37 PM Screenshot 2024-12-07 at 5 05 27 PM
Project accordion Screenshot 2024-12-07 at 5 05 08 PM Screenshot 2024-12-07 at 5 04 59 PM
Link Screenshot 2024-12-07 at 5 04 31 PM Screenshot 2024-12-07 at 5 04 21 PM
Navbar Screenshot 2024-12-07 at 5 03 17 PM Screenshot 2024-12-07 at 5 03 27 PM
Button Screenshot 2024-12-07 at 5 03 01 PM Screenshot 2024-12-07 at 5 02 49 PM

@Bookie0 Bookie0 requested a review from a team as a code owner December 7, 2024 22:07
@dti-github-bot
Copy link
Member

dti-github-bot commented Dec 7, 2024

[diff-counting] Significant lines: 82.

@@ -59,7 +59,7 @@ const Navbar: React.FC = () => {
<div className="hidden !justify-self-end w-fit lg:inline-flex flex-row">
{navbarItems.map((item) => (
<a
className={`hover:underline cursor-pointer text-white p-4 underline-offset-8 decoration-2 decoration-white ${
className={`hover:underline cursor-pointer text-white p-4 underline-offset-8 decoration-2 decoration-white h-[48px] flex items-center ${
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to make links same height as logo and vertically center the labels within the link wrapper

@@ -240,3 +265,32 @@ summary::after {
details[open] > summary::after {
rotate: 180deg;
}


.custom-focus-state:focus-visible {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the custom-focus-state class basically removes the default focus state outline on the element and instead places it on an absolutely positioned ::after pseudo-element so that it looks visually better

@@ -314,7 +314,7 @@ const MemberGroup: React.FC<MemberGroupProps> = ({
<>
<button
onClick={() => onMemberCardClick(member)}
className="memberCard"
className="memberCard custom-focus-state"
Copy link
Collaborator Author

@Bookie0 Bookie0 Dec 7, 2024

Choose a reason for hiding this comment

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

see what custom-focus-state does here

@Bookie0 Bookie0 requested a review from andrew032011 December 7, 2024 23:08
@Bookie0
Copy link
Collaborator Author

Bookie0 commented Dec 7, 2024

why failing

😭
Screenshot 2024-12-07 at 6 09 03 PM

@andrew032011
Copy link
Collaborator

andrew032011 commented Dec 7, 2024

why failing

😭 Screenshot 2024-12-07 at 6 09 03 PM

It's just failing to find Inter. That's known to be flaky sometimes. You can ignore it

@andrew032011
Copy link
Collaborator

Screenshot 2024-12-07 at 6 18 40 PM
Focus state on the team carousel looks a little funny.

@andrew032011
Copy link
Collaborator

on apply page i think the focus state doesn't show up for the "Don't know who to chat with?" button, is it because it's black? do we want a different color?
Screenshot 2024-12-07 at 6 20 43 PM

Copy link
Collaborator

@andrew032011 andrew032011 left a comment

Choose a reason for hiding this comment

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

the changes you made look good and I tabbed my way through every page of the website! LMK what u think about the other two cases though

@Bookie0
Copy link
Collaborator Author

Bookie0 commented Dec 7, 2024

on apply page i think the focus state doesn't show up for the "Don't know who to chat with?" button, is it because it's black? do we want a different color?

ya it should have secondary styling with red border
im fixing that one in another PR

once it gets the new styling, the proper focus state (red outline) should appear

@Bookie0
Copy link
Collaborator Author

Bookie0 commented Dec 7, 2024

the team carousel looks a little funny.

oops ok i'll fix that

@Bookie0 Bookie0 requested a review from andrew032011 December 8, 2024 00:19
@Bookie0 Bookie0 merged commit 666adac into main Dec 8, 2024
5 checks passed
@Bookie0 Bookie0 deleted the clement-custom-focus-states branch December 8, 2024 00:24
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.

3 participants