search

frontendstudygroup / frontendstudygroup.github.io

Web application for the frontend study group from WomenWhoCode Frontend track.
https://frontendstudygroup.github.io/
17 stars 17 forks source link

Fronenddesign #24

Closed Priyami closed 3 years ago

Priyami commented 3 years ago
princiya commented 3 years ago

@Priyami thanks for the PR. I will review tomorrow since it's already late night here for me :). Have a nice rest of the weekend.

princiya commented 3 years ago

@Priyami I am afraid we cannot accept this PR to be merged. Looks like you didn't claim any issue, so it has led to redundant work.

For example: @RosellaFer is working on header (issue #14) and added react-dom-router in this PR #23 . She has already claimed issue #14 and would be working on the card layout. You could pick up #20 or create new issues for filter, search (and claim these issues) like discussed here.

Please claim an issue before working on a PR so that others know that it's being assigned to you and you are working on it :).

princiya commented 3 years ago

Another comment, we discussed in the slack study group about the following:

avoid using any 3rd party libraries. the goal is to study and learn things. so if you have a favorite library, try to implement that functionality yourself from scratch.

I see from your PR you have used react-bootstrap, popper.js etc. Let's not use any such 3rd parties and instead use this repository to code things from scratch. If we use 3rd parties, each one will have their own preferences, like bootstrap vs tailwind etc. Let me know if you have any feedback or comments on this.

Priyami commented 3 years ago

I am so amateur in this GitHub world. Apologize for this PR. I will claim the issue and look into @RosellaFer work on the header (issue #14). I have installed the 3rd party libraries since I got some warning about popper.js is required. Should I ignore a warning like this in the future? - npm WARN bootstrap@4.4.1 requires a peer of popper.js@^1.16.0 but none is installed.

princiya commented 3 years ago

@Priyami no need to apologise, we are all learning together in this, all good :) the idea of this repo is to get comfortable working in the open, so I will encourage active and open communication. regarding your warning, yes those warnings can be ignored. but we will not need bootstrap too.

princiya commented 3 years ago

closing this PR since this is not valid anymore given that all changes were implemented individually in other PRs.