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

Feature: Team Page #4125

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Feature: Team Page #4125

merged 1 commit into from
Sep 21, 2023

Conversation

KevinMulhern
Copy link
Member

@KevinMulhern KevinMulhern commented Sep 10, 2023

Because:

This commit:

  • Adds a team locales file where team member details will live
  • Divides the page into the team role structure we have within TOP.
  • Adds an alumni section for former team members.
  • Adds a smooth scroll anchor links for easier navigation between roles.

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 10, 2023 14:12 Inactive
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 10, 2023 14:19 Inactive
@KevinMulhern
Copy link
Member Author

This is still a WIP - but I'm open for early feedback https://odin-review-app-pr-4125.herokuapp.com/team

@ManonLef
Copy link
Member

Love the overall look but will have to check on desktop! Noticed something funny with social link icons on mobile though.

IMG_5435

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 11, 2023 09:49 Inactive
@KevinMulhern
Copy link
Member Author

Good spot, thanks @ManonLef 💪

@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from dc9b3a5 to 2660508 Compare September 11, 2023 19:23
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 11, 2023 19:23 Inactive
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 11, 2023 20:54 Inactive
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 12, 2023 09:33 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from c8b5e40 to 8108587 Compare September 12, 2023 09:42
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 12, 2023 09:42 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 8108587 to 69777d7 Compare September 12, 2023 09:57
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 12, 2023 09:57 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 69777d7 to 43a851c Compare September 13, 2023 08:11
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 13, 2023 08:11 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 43a851c to d46c24a Compare September 13, 2023 08:16
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 13, 2023 08:16 Inactive
@KevinMulhern KevinMulhern marked this pull request as ready for review September 13, 2023 08:50
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from d46c24a to 72f58e5 Compare September 14, 2023 07:57
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 14, 2023 07:57 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 72f58e5 to 6e7ee88 Compare September 14, 2023 08:02
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 14, 2023 08:02 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 6e7ee88 to 097b219 Compare September 14, 2023 08:10
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 14, 2023 08:10 Inactive
@KevinMulhern
Copy link
Member Author

The team footer link has been removed so this can be merged without all the team data populated first. We'll then do small follow up PR's adding everyone to the team yaml file. To access the page on the review app, you can go directly to the team page url: https://odin-review-app-pr-4125.herokuapp.com/team

@KevinMulhern KevinMulhern added the Type: Enhancement Involves a new feature or enhancement request label Sep 14, 2023
@KevinMulhern KevinMulhern self-assigned this Sep 14, 2023
@KevinMulhern KevinMulhern requested a review from a team September 14, 2023 20:30
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 16, 2023 16:40 Inactive
@thatblindgeye
Copy link
Contributor

Copy pasting some feedback from Discord:

  • For the user avatars, we could probably get away with an empty "" alt attribute, since they seem more decorative than anything and not really important for AT to be able to traverse
  • Could be worth adding an aria-labelledby or aria-label on the ul elements, both the list of all members of each team as well as the list of each person's personal links, so that the lists are more descriptive, e.g. "List Core team 7 items" and "List Corbin Dinis social media 2 items" etc
  • For the link icons for each member card, I feel like we should include the user name as part of the hidden link text, e.g. "Corbin Dinis GitHub" (or "Corbis Dinish on GitHub" or something), just so there aren't a ton of generic "GitHub" link texts on the page. Either that, or making that hidden link text aria-hidden="true", add an aria-label="[site]" to the anchor element, add an ID to the anchor and the heading with the username, and apply an aria-lablledby to the anchor element. The latter would result in less duplicate text if the styles don't load properly while still announcing links more uniquely. So basically:
<h3 id="some-id-1">
...
<a aria-labelledby="some-id-1 some-id-2" id="some-id-2" aria-label="Twitter">
...
<span class="sr-only" aria-hidden="true">

@KevinMulhern
Copy link
Member Author

KevinMulhern commented Sep 17, 2023

Thanks @thatblindgeye, I have points 1 and 3 done.

For labelling the ul elements, is that a common thing? I haven't seen that before, but I'd love to read up on it more if you have any suggested resources? I've only really seen role and listitem used in regards to non typical lists and accessibility.

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 17, 2023 15:08 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 3c44425 to 012c907 Compare September 17, 2023 15:13
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 17, 2023 15:13 Inactive
@ManonLef
Copy link
Member

ManonLef commented Sep 18, 2023

Maybe a bit late, especially considering this PR being open as well... But I might think the rounding is a bit too much.
I can't play around with the deployment anymore. But perhaps we should try the same rounding as the path sections which is rounded-lg?

Screenshots below
Screenshot 2023-09-18 at 20 59 18
Screenshot 2023-09-18 at 20 59 31

@ManonLef
Copy link
Member

ManonLef commented Sep 18, 2023

I edited the first card on the homepage to be rounded-lg of the previously mentioned PR which adds rounding to the cards:

Screenshot 2023-09-18 at 21 05 12

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 18, 2023 19:25 Inactive
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 012c907 to 2591c72 Compare September 18, 2023 19:30
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 18, 2023 19:30 Inactive
@ManonLef
Copy link
Member

ManonLef commented Sep 18, 2023

rounded-lg

Screenshot 2023-09-18 at 21 38 09

rounded-xl

Screenshot 2023-09-18 at 21 38 15

@KevinMulhern
Copy link
Member Author

Thanks for the feedback @manon, putting them side by side, I prefer the larger border radius on this page. It has a better fit and feel imo.

rounded-lg
Screenshot 2023-09-18 at 20 16 26

rounded-2xl
Screenshot 2023-09-18 at 20 15 55

Although, I think you're right about rounded-lg fitting better with the home page course cards.

I've kicked off a deploy so you can have a play?

@ManonLef
Copy link
Member

Yes I was playing as soon as I saw that deploy 😄

If you like it better, you should leave it!
It looks like you lost some top padding for some reason in your screenshots btw.
Screenshot 2023-09-18 at 21 54 09

@KevinMulhern
Copy link
Member Author

Good spot 🦅 👁️

I tweaked the top padding, member cards with no social links were looking a bit top heavy.
It's dangerous to look at things like this after having a break from it for a few days 😆

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 20, 2023 08:51 Inactive
Copy link
Member

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

I've got one question in a few places, other than that looks awesome, killer work on this 💪


<% if team_member.socials.any? %>
<ul
aria-labelledby="List of <%= team_member.name %> social links <%= team_member.socials.size %> items"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be aria-label rather than aria-labelledby since we're passing a description rather than an id?

</div>

<ul
aria-labelledby="<%= role %> list, <%= t("team_members.#{role}.members").size %> members"
Copy link
Member

Choose a reason for hiding this comment

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

Same labelledby vs label question here.

</div>

<ul
aria-labelledby="<%= role %> team list, <%= t("team_members.#{role}.members").size %> members"
Copy link
Member

Choose a reason for hiding this comment

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

And here.

@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 2591c72 to 64230fe Compare September 20, 2023 20:49
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 20, 2023 20:50 Inactive
@KevinMulhern
Copy link
Member Author

Thanks @wise-king-sullyman, I got those changed over. The feedback is much appreciated mate.

I really need to brush up on accessibility 😅

@wise-king-sullyman
Copy link
Member

Haha pretty much all credit for any a11y knowledge I have goes to @thatblindgeye

Copy link
Member

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

🔥 🔥 🔥

@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 64230fe to 6d6cc87 Compare September 21, 2023 08:08
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 21, 2023 08:08 Inactive
Because:
* TheOdinProject/top-meta#3
* Replaces the [hall of fame on the contributing page](http://theodinproject.com/contributing)

This commit:
* Adds a team locales file where team member details will live
* Divides the page into the team role structure we have within TOP.
* Adds an alumni section for former team members.
* Adds a smooth scroll anchor links for easier navigation between roles.
@KevinMulhern KevinMulhern force-pushed the feature/odin-team-page-b branch from 6d6cc87 to 34f3bc4 Compare September 21, 2023 08:14
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4125 September 21, 2023 08:15 Inactive
@KevinMulhern KevinMulhern merged commit 83d72eb into main Sep 21, 2023
@KevinMulhern KevinMulhern deleted the feature/odin-team-page-b branch September 21, 2023 08:21
@thatblindgeye
Copy link
Contributor

@KevinMulhern I don't have any resources on hand at the moment, but essentially by providing a label:

  • It provides more context if a user is traversing the lists on a page via keyboard shortcut with assistive tech (or via a rotor menu, if it has a category for lists)
  • Provides context when a user is normally traversing a page. "List, 2 items" vs "Social media, list, 2 items", the user knows what exactly the purpose/content of the list is/meant to be immediately (which that may be more apparent in certain situations, but still)
  • It differentiates from other lists on the page if there are several (ties back to keyboard shortcuts and rotor menu)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Involves a new feature or enhancement request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants