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

Fahim/Tony #39 #47

Merged
merged 7 commits into from
Nov 25, 2024
Merged

Fahim/Tony #39 #47

merged 7 commits into from
Nov 25, 2024

Conversation

tony1759
Copy link
Collaborator

@tony1759 tony1759 commented Nov 16, 2024

Describe your changes

Implemented proper gradient banner layout
Added student enrollment indicators
Styled class cards with proper spacing and hover effects
Updated Button component to use design system colors
Added "Other Levels" section with proper styling
Improved component modularity following CDD principles

Issue ticket number and link

#39

Does your code meet the acceptance criteria?

  • [] Yes

Testing done and screenshots (if relevant)

Navigation between different levels
Responsive layout on mobile and desktop
Button hover states and transitions
Loading states
Error handling for missing data
Screenshot 2024-11-16 at 10 41 32 AM

Other comments

Copy link
Collaborator

@myix765 myix765 left a comment

Choose a reason for hiding this comment

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

The Classes page is looking really good! Just wanted to request a few changes to make the page look more like the high fidelity Figma design.

  • For the gradient could you change the color at the end of the gradient to be a darker blue? You don't have to get the colors exactly right, but right now both gradient colors look almost the same.
  • In the Figma it looks like the grid for the classes has 4 columns, not 2
  • Right now the levels navigation on the bottom is not using the Levels component, so the blue strip you've added is missing. Please update the Other Levels section to use the Level component. Also, for the blue strip, if you notice on the Figma the way that's done is by using an inset border only on the left side. We can do the same thing here using the box-shadow CSS rule. For more information on using box-shadow, you can look at this site which also has a demo area where you can play around with different values (especially the fourth one with the inset rule). Then to apply box-shadow with Tailwind, you can check this page in the docs which shows how to use arbitrary values to create your own custom box-shadow.

Let me know if you have any questions!

@epicfahimxd epicfahimxd requested a review from myix765 November 18, 2024 22:21
src/pages/Classes.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@lillian-tran lillian-tran left a comment

Choose a reason for hiding this comment

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

Hi guys - thank you for the awesome progress! In addition to the last couple things Megan mentioned, please also make small adjustments to the navbar (ordering of the pages, hover states/coloring, etc.) to make it closer to the Figma. You can leave the dropdown for the language as a click to drop instead of hover for now.

@myix765 myix765 merged commit 2dfd20f into main Nov 25, 2024
1 check passed
@myix765 myix765 deleted the high-fidelity-sections branch November 25, 2024 18:42
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.

4 participants