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

implemented designs for application process section #231

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

Conversation

dersual
Copy link
Contributor

@dersual dersual commented Nov 20, 2024

#216

Implemented new designs for the application process section and internal teams sections.
image
image

@dersual dersual linked an issue Nov 20, 2024 that may be closed by this pull request
2 tasks
@dersual
Copy link
Contributor Author

dersual commented Nov 20, 2024

Have to fix merge issues, will be done later.

src/_includes/_layouts/ForStudents.jsx Outdated Show resolved Hide resolved
src/_includes/_layouts/ForStudents.jsx Show resolved Hide resolved
@Caposto
Copy link
Contributor

Caposto commented Nov 22, 2024

Can you add your components to a ForStudents directory in the components folder? Can you also group the StudentsHero component into it as well and update the imports?

@miguel-merlin miguel-merlin requested a review from Caposto December 5, 2024 00:35
@miguel-merlin
Copy link
Member

Small comment for future reference. You are supposed to name your branch in the following format:

  • feature/{name_of_feature} if you are adding a new feature
  • `fix/{name_of_fix} if you are fixing some bug
  • docs/{name_of_docs} if you are adding documentation


export default function ApplicationProcess({ comp }) {
return (
<section className="flex flex-col items-center px-12 pb-12 max-lg:px-12 max-md:w-full max-md:px-4">
Copy link
Member

Choose a reason for hiding this comment

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

nit: The top padding of the application changes does not correspond. to the paddings elsewhere in the /students page.
image
image
Try to fix the padding, so that they are both the same. You will need to change the top padding in this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are talking about the weird spacing between the Our Teams and Application Process sections, it's because the component/element before it has a large padding at the bottom. Although I didn't make that component, would you like me to adjust the padding?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a commit fixing the bottom padding of the abovementioned component, though the extra space can also be attributed to its weird height. I talked to Christian yesterday and it should be okay for now, just wanted to check if the spacing is also okay for you now.

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.

Students - Application Process + Internal Teams
3 participants