🏗️ Pop ✍️ up Comment window and Involvements 👍 #32

Zia123456 · 2022-11-26T14:35:24Z

🚧 ATTENTION 🚧

The falling checks are for the API key which we decided to expose for the Reviewer testing as advised by the previous reviewer.

I this PR We implemented the following:

Thanks for the review 🥂

Display home page

⚙️ 🧞 Testing with Jest capstone

zieeco

Hi @Team,

Great job so far! 👏
You've done well, however, there are some issues that you still need to work on to go to the next project, but you are almost there!

To highlight 🎯

No linter errors.✔️
Nice design. ✔️
Descriptive PR. ✔️

Required Changes ♻️

Check the comments under the review.

Kindly address those issues so that the next reviewer can review your project properly as all the issues I found made me not able to make appropriate review on the project. 🙏

Optional suggestions

N/A

Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.

Cheers and Happy coding!👏👏👏

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.

Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.

As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.

zieeco · 2022-11-26T15:29:55Z

src/modules/comments-api.js

+/* eslint-disable import/no-unresolved */
+/* eslint-disable operator-linebreak */


REMOVE ALL LINTER IN THE PROJECT AND FIX THEM

The linter disabled is way too much and it makes your codebase not clean and readable

Please, let's remove all the eslint disable command and fix the linter error to improve our abilities to write clean and maintainable codebases that conforms to the linter as this is widely used in software development to ensure good code quality.

zieeco · 2022-11-26T15:32:53Z

src/modules/comments-api.js

@@ -0,0 +1,36 @@
+/* eslint-disable import/no-unresolved */
+/* eslint-disable operator-linebreak */
+import { key } from './api-key.js';


Hey team, I'm finding it difficult to run your app as I think the imported import { key } from './api-key.js'; does not exist in your codebase, Please, kindly double check as this is throwing an error and make your app not to run.

As a result of this, I can't make appropriate reviews on the project. Please, kindly fix this.

We avoided pushing the API keys into GitHub for security purposes.
Set up instructions was included n the read me
@zieeco

zieeco · 2022-11-26T15:38:26Z

src/modules/fetch.js

+// import renderPopup from './popup.js';
 /* eslint-disable camelcase */
-// hold our api in a constant
+// eslint-disable-next-line import/no-mutable-exports


As this is a JavaScript project, the naming convention used in JS is camelCase, so why not try following best practices than disabling the linter? Please, remove all the linter disabled commands and fix the issue.

zieeco · 2022-11-26T15:42:53Z

src/modules/fetchLikes.js

+/* eslint-disable operator-linebreak */
+/* eslint-disable consistent-return */
+/* eslint-disable camelcase */
+/* eslint-disable import/no-unresolved */
+/* eslint-disable no-unused-vars */


Hey Team, most of these linters are pretty easy to fix them. Please, let's try fixing them to make our codebase clean and maintainable.

NOTE: I understand that you're behind schedule for the submission of this project, however, if you can't fix it, kindly state the reason why you couldn't and the next reviewer would overlook it

Implemented @zieeco

zieeco · 2022-11-26T15:44:26Z

src/modules/newLike.js

+/* eslint-disable consistent-return */
+/* eslint-disable no-useless-concat */


Please, let's try and fix the linter to improve our ability on writing clean readable code.

zieeco · 2022-11-26T15:46:31Z

src/modules/newLike.js

+/* eslint-disable import/no-unresolved */
+import { API_KEY } from './api-key.js';


You disabled the linter here because eslint can't find this import { API_KEY } from './api-key.js'; anywhere is the directories/files. please, kindly check if it exists to fix the linter error.

zieeco · 2022-11-26T15:50:39Z

README.md

@@ -15,7 +15,6 @@
 - Webpack
 - Jest - for Unit Testing.

-#### [Live link](https://simongideon.github.io/javascript-group-capstone-project/)


[REQUIRED CHANGES ON README FILE]

Hey, great job with your readme, however, the microverse readme template has been updated to this current version. Do kindly update your readme to conform to these changes and also, customize it to suit your current project.

Great job adding the live demo version but kindly note that you're deployment is not showing your project. You can try using netlify to deploy the app. I addition, you're yet to add a video presentation as this is a core requirement to have your project approved. please, refer to the project requirement to add this missing part of the requirement.

You should also add the link to the video just like the live demo version

…oup-capstone-project into pop-up

gitguardian · 2022-11-26T17:16:10Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	Secret	Commit	Filename
5102819	Generic High Entropy Secret	`820758f`	src/modules/api-key.js	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

Ismailco

Hi @Zia123456, @SimonGideon,

In my opinion, your project is ready for presentation! There is nothing else to say other than... it's time to merge it

Highlights

✔️ No linter errors
✔️ Used Gitflow
✔️ Work documented professionally

Good luck! 🎉

Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.

Ismailco · 2022-11-26T18:20:33Z

src/modules/newLike.js

@@ -0,0 +1,24 @@
+/* eslint-disable no-useless-concat */


[OPTIONAL] Please remove all the disabled linters and fix the linters for better practices and clean code, I will not require you to change them because you fixed the specifically pointed linters by the previous reviewer, this is the last review you have, and the deadline is near however I highly recommend fixing this linters instead of disabling them

Zia123456 and others added 30 commits November 22, 2022 21:18

popup added

782b4a3

comment section done

2d2d713

Merge pull request #24 from SimonGideon/display-home-page

66200b5

Display home page

Merge remote-tracking branch 'origin/merge-solve' into pop-up

d6658c1

Final touch on likes

974c5c9

Styling

2801f48

Stylings

6f956f5

Initial commit

03e88f7

intial commit

61de31e

Initial commit

12379cb

Finals commit

0aafd9d

popup created

e64f2fe

Testing with jest

519e504

Create linters.yml

83f1135

Soleve linters

80ebe33

fine-tune

1875e8b

Refactor stylings

659d509

IX linters

fae6b05

FIX linters

f632efb

Initial commit

ff88385

Create linters.yml

acebc8e

SOLED linters

6f4d421

Merge pull request #29 from SimonGideon/testing-capstone

c81c478

⚙️ 🧞 Testing with Jest capstone

Update README.md

023c3a0

Update README.md

8d10b24

comment counter and test created

578ebca

some changes

156d1c9

conflict resolved

6d72abe

some design added

492453e

some css added

d6bfd37

github-pages bot temporarily deployed to github-pages November 26, 2022 15:11 Inactive

Update README.md

e59b808

github-pages bot temporarily deployed to github-pages November 26, 2022 15:16 Inactive

Update README.md

1d12806

github-pages bot temporarily deployed to github-pages November 26, 2022 15:17 Inactive

Merge branch 'development' into pop-up

7955e50

github-pages bot temporarily deployed to github-pages November 26, 2022 15:18 Inactive

zieeco suggested changes Nov 26, 2022

View reviewed changes

Update README.md

37448f8

github-pages bot temporarily deployed to github-pages November 26, 2022 17:11 Inactive

Zia123456 added 2 commits November 26, 2022 09:13

linter comments removed

820758f

Merge branch 'pop-up' of https://github.com/SimonGideon/javascript-gr…

4c3ccc0

…oup-capstone-project into pop-up

github-pages bot temporarily deployed to github-pages November 26, 2022 17:15 Inactive

github-pages bot temporarily deployed to github-pages November 26, 2022 17:16 Inactive

project built

44afa58

github-pages bot temporarily deployed to github-pages November 26, 2022 17:21 Inactive

Update README.md

3e0af4e

github-pages bot temporarily deployed to github-pages November 26, 2022 17:47 Inactive

Update README.md

61a4459

github-pages bot temporarily deployed to github-pages November 26, 2022 17:50 Inactive

conflict resolved

600c76b

github-pages bot temporarily deployed to github-pages November 26, 2022 18:02 Inactive

Ismailco approved these changes Nov 26, 2022

View reviewed changes

Update README.md

f377c6a

github-pages bot temporarily deployed to github-pages November 26, 2022 18:33 Inactive

Update README.md

6e25414

SimonGideon merged commit 62dc9cb into development Nov 26, 2022

github-pages bot deployed to github-pages November 26, 2022 18:44 View deployment

SimonGideon temporarily deployed to github-pages July 5, 2023 19:00 — with GitHub Pages Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗️ Pop ✍️ up Comment window and Involvements 👍 #32

🏗️ Pop ✍️ up Comment window and Involvements 👍 #32

Zia123456 commented Nov 26, 2022 •

edited by SimonGideon

Loading

zieeco left a comment •

edited by Zia123456

Loading

zieeco Nov 26, 2022 •

edited by Zia123456

Loading

zieeco Nov 26, 2022

SimonGideon Nov 26, 2022

zieeco Nov 26, 2022

SimonGideon Nov 26, 2022

zieeco Nov 26, 2022

SimonGideon Nov 26, 2022

zieeco Nov 26, 2022

zieeco Nov 26, 2022

zieeco Nov 26, 2022

gitguardian bot commented Nov 26, 2022 •

edited

Loading

Ismailco left a comment

Ismailco Nov 26, 2022

		/* eslint-disable import/no-unresolved */
		/* eslint-disable operator-linebreak */

		/* eslint-disable consistent-return */
		/* eslint-disable no-useless-concat */

		/* eslint-disable import/no-unresolved */
		import { API_KEY } from './api-key.js';

🏗️ Pop ✍️ up Comment window and Involvements 👍 #32

🏗️ Pop ✍️ up Comment window and Involvements 👍 #32

Conversation

Zia123456 commented Nov 26, 2022 • edited by SimonGideon Loading

🚧 ATTENTION 🚧

I this PR We implemented the following:

zieeco left a comment • edited by Zia123456 Loading

Choose a reason for hiding this comment

To highlight 🎯

Required Changes ♻️

Optional suggestions

zieeco Nov 26, 2022 • edited by Zia123456 Loading

Choose a reason for hiding this comment

REMOVE ALL LINTER IN THE PROJECT AND FIX THEM

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented Nov 26, 2022 • edited Loading

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Ismailco left a comment

Choose a reason for hiding this comment

Hi @Zia123456, @SimonGideon,

Highlights

Choose a reason for hiding this comment

Zia123456 commented Nov 26, 2022 •

edited by SimonGideon

Loading

zieeco left a comment •

edited by Zia123456

Loading

zieeco Nov 26, 2022 •

edited by Zia123456

Loading

gitguardian bot commented Nov 26, 2022 •

edited

Loading