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

Jump to bottom

524 - initail changes, added Alumni tab, reusable components #536

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

524 - initail changes, added Alumni tab, reusable components #536

wants to merge 8 commits into from
+141 −120

Conversation

Sukritib13
Copy link
Contributor

@Sukritib13 Sukritib13 commented Aug 31, 2024 •
edited
Loading

Changes -

  • Added Alumni tab, used lime colour (The 3rd color in techstart symbol) for LinkedIn icon, Executive tab is still default
  • Made code reusable by making it into 3 buttons and using switch statements
  • Cleaned up some CSS

@Sukritib13 Sukritib13 linked an issue Aug 31, 2024 that may be closed by this pull request
Add Alumni Section on our Team Page for Janita and Sajwal #524
Open
@vercel Vercel
Copy link

vercel bot commented Aug 31, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tsu-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 11:34pm

ParsaKargari
ParsaKargari requested changes Aug 31, 2024
View reviewed changes
Copy link
Contributor

@ParsaKargari ParsaKargari left a comment

Choose a reason for hiding this comment

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

I like the idea of turning it into a component, but I'd change the UI design back to the slider we had. Also watch out for the responsiveness of the component, it doesn't look like it's responsive for mobile users / small-screen users.
image
image

src/components/TeamSection/Team.tsx Outdated Show resolved Hide resolved
src/components/TeamSection/teamMembers.json Outdated Show resolved Hide resolved
Anidion
Anidion requested changes Sep 10, 2024
View reviewed changes
Copy link
Collaborator

@Anidion Anidion left a comment

Choose a reason for hiding this comment

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

Great work, this is looking good. Mainly code style feedback to look at.
Could you also add a transition for the background color?
Just background-color ease-in-out 0.2s would be enough

And on mobile, text should be centered vertically and the slider should have even spacing on both sides:
image

src/components/TeamSection/Profile.tsx Outdated Show resolved Hide resolved
src/components/TeamSection/Team.tsx Outdated Show resolved Hide resolved
src/components/TeamSection/TeamSection.styles.ts Outdated Show resolved Hide resolved
src/components/TeamSection/TeamSection.styles.ts Outdated Show resolved Hide resolved
src/components/TeamSection/teamMembers.json Outdated Show resolved Hide resolved
src/utility/SharedStyles.tsx Outdated Show resolved Hide resolved
@Sukritib13
Addressing comments - creating helper functions, fixing styles
b6a1000 
Signed-off-by: Sukritib13 <113867416+Sukritib13@users.noreply.github.com>
@Sukritib13
addressing comments - using var for bg color, using type for readability
041715f 
Signed-off-by: Sukritib13 <113867416+Sukritib13@users.noreply.github.com>
@brian-ngyn
Copy link
Member

Looks good, a few more comments + a rebase and should be good to go in

@Sukritib13
using types and correcting case
b8eb6ce 
Signed-off-by: Sukritib13 <113867416+Sukritib13@users.noreply.github.com>
@Sukritib13
using memoization
dac25e9 
Signed-off-by: Sukritib13 <113867416+Sukritib13@users.noreply.github.com>
Anidion
Anidion approved these changes Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@Anidion Anidion left a comment

Choose a reason for hiding this comment

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

Awesome work 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anidion Anidion Anidion approved these changes

@brian-ngyn brian-ngyn brian-ngyn approved these changes

@Sahitiakella Sahitiakella Awaiting requested review from Sahitiakella

@IsaiahA21 IsaiahA21 Awaiting requested review from IsaiahA21

@mortezafa mortezafa Awaiting requested review from mortezafa

@ParsaKargari ParsaKargari Awaiting requested review from ParsaKargari

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Alumni Section on our Team Page for Janita and Sajwal
4 participants
@Sukritib13 @brian-ngyn @Anidion @ParsaKargari