search

redhat-beyond / JobSeeker

https://github.com/redhat-beyond/JobSeeker
MIT License
2 stars 5 forks source link

🏇 Feed: Adding a Webpage #73

Closed paOmer closed 2 years ago

paOmer commented 2 years ago

Adding a feed webpage that shows all the existing posts.

Screen Shot 2022-04-09 at 15 13 00

Close #72

paOmer commented 2 years ago

I think this is a great start for the look of the feed and the additional tests- very well done!

I have a couple of suggestions, probably not relevant to this PR- for the improvement of the UI later on (in my opinion):

  • Make the buttons of like and comment with only the logos, would look cleaner.
  • Add a search button- if not too hard to implement I think would be a valuable addition.
  • If possible- make the posts bigger in proportion to the page.

Please tell me what you think, if you like some of the suggestions I'll be happy to open relevant issues.

@ShirleyFichman Thanks for reviewing, I've maid the posts bigger as you suggested (you can view the new screenshot at the PR description). About the search bar, I think its a great idea to try and implement in the future if the time will allow. Regarding to the buttons description , I tried to remove it but I think it looks better with the description on, I'll add a screenshot for you to look at and share your thoughts.

Screen Shot 2022-04-09 at 15 05 49
paOmer commented 2 years ago

Please split the this PR to 3 different PRs

Hi @Yarboa, please notice that in the PR description I've mentioned that the first 2 commits (which include 6 files) are from an earlier PR, so dont mind them. That means this PR only deals with 5 files, and does very minor changes for some of them. I believe it should stay as a single PR since all the changes have the same one purpose. Please let me know your thoughts about it, thanks ahead!

Yarboa commented 2 years ago

Please split the this PR to 3 different PRs

Hi @Yarboa, please notice that in the PR description I've mentioned that the first 2 commits (which include 6 files) are from an earlier PR, so dont mind them. That means this PR only deals with 5 files, and does very minor changes for some of them. I believe it should stay as a single PR since all the changes have the same one purpose. Please let me know your thoughts about it, thanks ahead!

Sorry you have models and UI in the same PR. Please consider sharing this work with @DeanBiton one of you can submit the model the other the UI

paOmer commented 2 years ago

Please split the this PR to 3 different PRs

Hi @Yarboa, please notice that in the PR description I've mentioned that the first 2 commits (which include 6 files) are from an earlier PR, so dont mind them. That means this PR only deals with 5 files, and does very minor changes for some of them. I believe it should stay as a single PR since all the changes have the same one purpose. Please let me know your thoughts about it, thanks ahead!

Sorry you have models and UI in the same PR. Please consider sharing this work with @DeanBiton one of you can submit the model the other the UI

We splitted the PR. @DeanBiton made the tests at PR #87. This PR still has a test update, due to feed page loading posts from DB, I had to give the entrypoint access to DB to make sure the test will pass, another option will be to build this PR from #64 and make this one depends on it. If you think the other way is preferred, please let me know and I'll update accordingly.

Yarboa commented 2 years ago

LGTM