search

jslovers / jslovers-official-website

JSLovers Official Website built on Next.js
MIT License
53 stars 47 forks source link

Introduce CarouselWithDots component, enhance responsiveness on about, job, and meetup pages, and modified follow us section in footer #90

Open MS07062000 opened 1 week ago

MS07062000 commented 1 week ago

Type: Feat (Feature) Category: UI Components Refactor in Homepage Scope: Carousel and TalkCard Components inside desktop and Mobile Design Task: Component Refactoring and Type Definition Cleanup

Description

Expected

image

Snapshots/Videos: image image image

Type of change

Added tests?

Internationalization Support?

Steps to test the feature:

  1. Ensure the CarouselWithDots component appears correctly on pages requiring carousel navigation.
  2. Verify dot navigation functionality by interacting with the dots to navigate through carousel items.
  3. Check that TalkCard components display correctly with updated naming and adhere to the Talk type structure.

Snapshot of the test-cases that are passing: As this PR involves static updates and component refactoring, there are no specific test cases to validate programmatically.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) No, this PR does not introduce breaking changes. Users should update import paths to CarouselWithDots and TalkCard components if they were using the previous Talk component.

MS07062000 commented 1 week ago

Changes Made:

Expected:

image

Snapshots/Videos:

image image

The mobile view may appear suboptimal due to issues with the display of job cards.

MS07062000 commented 1 week ago

Changes Made:

Snapshots: image image image

The job page should now be responsive with these updates.

MS07062000 commented 1 week ago

Changes Made:

Snapshots:

image image image image

These changes enhance the visual consistency and functionality of the application, aligning it closer to design expectations .

Neha commented 1 week ago

Changes Made:

  • Revamped job filters' mock data.
  • Split the job section into a distinct component.
  • Implemented a filter component following Figma's specifications and removed a previously present dropdown that did not align with the Figma design.
  • Added new types and interfaces customized for the job section.

Expected:

image

Snapshots/Videos:

image image

The mobile view may appear suboptimal due to issues with the display of job cards.

This is good. One request: Please create a new PR, and if there is already an issue for this. Please assign that issue and do the needful. If not, please create one and wait for triage. Please do not mix the features in one PR>

Neha commented 1 week ago

Changes Made:

  • Modified styles using TailwindCSS in Jobcard.
  • Implemented CarouselWithDots for media widths less than large screens in Jobs page.
  • Commented out unnecessary code related to similar jobs in Jobsection; this code is needed in job description pages only.
  • Added a prop className to CarouselWithDots to hide the carousel on large devices and allow for further styles customization.
  • Uncommented a card related to learning for interviews as per Figma design in job section and commented out SignupForMeetupCard component.
  • Adjusted jslovers logo width from 150 to 100 to eliminate unnecessary space.

Snapshots: image image image

The job page should now be responsive with these updates.

Question: Why the footer is wrapping next to job card? This is not how it should be. Is it from Figma?

Commented out unnecessary code related to similar jobs in Jobsection; this code is needed in job description pages only. Added a prop className to CarouselWithDots to hide the carousel on large devices and allow for further styles customization. - Do not comment the code. Delete the not required code.

Uncommented a card related to learning for interviews as per Figma design in job section and commented out SignupForMeetupCard component. - SignupForMeetupCard was component required to have consistency across all pages. Figma is not updated. Please undo the changes.

Adjusted jslovers logo width from 150 to 100 to eliminate unnecessary space. - I am not sure about this. Can you please point to extra space are you referring too?

MS07062000 commented 1 week ago

Changes Made:

  • Modified styles using TailwindCSS in Jobcard.
  • Implemented CarouselWithDots for media widths less than large screens in Jobs page.
  • Commented out unnecessary code related to similar jobs in Jobsection; this code is needed in job description pages only.
  • Added a prop className to CarouselWithDots to hide the carousel on large devices and allow for further styles customization.
  • Uncommented a card related to learning for interviews as per Figma design in job section and commented out SignupForMeetupCard component.
  • Adjusted jslovers logo width from 150 to 100 to eliminate unnecessary space.

Snapshots: image image image The job page should now be responsive with these updates.

Question: Why the footer is wrapping next to job card? This is not how it should be. Is it from Figma?

Commented out unnecessary code related to similar jobs in Jobsection; this code is needed in job description pages only. Added a prop className to CarouselWithDots to hide the carousel on large devices and allow for further styles customization. - Do not comment the code. Delete the not required code.

Uncommented a card related to learning for interviews as per Figma design in job section and commented out SignupForMeetupCard component. - SignupForMeetupCard was component required to have consistency across all pages. Figma is not updated. Please undo the changes.

Adjusted jslovers logo width from 150 to 100 to eliminate unnecessary space. - I am not sure about this. Can you please point to extra space are you referring too?

In the Figma design, the job and meetup pages have separate cards, distinct from the 'SignupForMeetupCard'. I have adhered to all references from the Figma design.

I believe it's prudent to keep some code commented for now, as it might be used in another component later. If unnecessary, we can delete it at a later stage.

Currently, I am actively working on the Meetup detail page and making frequent modifications, so separating the PRs is not practical at the moment.

MS07062000 commented 1 week ago

Implemented Issues #8, #9, and #11 according to Figma design specifications. Partially resolved Issue #67 . Currently addressing Issue #14 .

Added social icons to the footer section, a new modification not previously present.

Neha commented 1 week ago

I appreciate your effort but would request you to work on the issues only when they are assigned to you. If someone is sitting on the issue , I try to every 2 weeks check with them and re-assign.

https://github.com/jslovers/jslovers-official-website/issues/14 : This is assigned to Ritesh

https://github.com/jslovers/jslovers-official-website/issues/67 : this is assigned to other people

https://github.com/jslovers/jslovers-official-website/issues/11 : this is assigned to someone else and PR is already there

https://github.com/jslovers/jslovers-official-website/issues/9 : This is assigned to someone else

https://github.com/jslovers/jslovers-official-website/issues/8 : This is also assigned to someone else.

I will go ahead and remove your PR from the above issues as it will confuse the current assigner.

Unfortunately, if you cannot break the PR I won't be to accept your PR. It is crucial to have a clean PR, and PR should have only the code/changes of the issue associated to it (1 issue -> 1 PR).

Reg commented code, GitHub can take care of history of the code. You don't need to worry about it. Always delete the code if not required.

MS07062000 commented 1 week ago

I noticed in the pull request 79 discussion that you requested all changes be included in the same PR, so I have followed that guideline accordingly.

Regarding the issue of separating commits made for each features into separate pull request, I may encountered difficulties as some changes required updates to previously pushed code. Nevertheless, I will make my one last commit to this PR for meetup detail page then I will not make any commit to this PR.

If the individuals whom you assigned to the issues do not respond. In such a case, you can review my PR instead.

Thank you for the opportunity to contribute to your website.

Additionally, I wanted to mention that Vercel offers a bot for free PR deployment. I encountered this feature in a previous project and thought it might be beneficial for deploying the dev branch of your website. This could provide a visual representation of the code changes more easily.

Neha commented 3 days ago

79

Thank you. Reg 79 PR, dev was closing his old open PR and opening new PR everytime he wanted to push a new change/or code review comment. As a result all the PR comments were getting lost. Hence, I guided him that he can push his code changes in the same PR (those were related to the only 1 issues he was working on).