Skip to content

Refactor listings/listings.jsx#6992

Merged
nickytonline merged 110 commits intoforem:masterfrom
JulianoGTZ:julianogtz/fixing-codeclimate-issue-on-listings
May 11, 2020
Merged

Refactor listings/listings.jsx#6992
nickytonline merged 110 commits intoforem:masterfrom
JulianoGTZ:julianogtz/fixing-codeclimate-issue-on-listings

Conversation

@JulianoGTZ
Copy link
Copy Markdown
Contributor

@JulianoGTZ JulianoGTZ commented Apr 1, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

I'm trying to solve the issues on codeclimate about the app/javascript/listings/listings.jsx file

I have failed to eliminate the code-smells 😢 . I thought it was better to dive into refactoring and then try to decrease code-smells in a more specific pull-request.

I increased the maintainability of the file from D to C.

I have done the tests by a BDD approach. Only on some small components that I did a snapshot test

Related Tickets & Documents

Refactor CodeClimate Issues

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added tests?

  • yes
  • no, because they aren't needed
  • no, because I need help

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Bart saying: it's hard to understand

@pr-triage pr-triage Bot added the PR: draft bot applied label for PR's that are a work in progress label Apr 1, 2020
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 1, 2020

CLA assistant check
All committers have signed the CLA.

@JulianoGTZ JulianoGTZ force-pushed the julianogtz/fixing-codeclimate-issue-on-listings branch from 08d4c00 to 4cb3758 Compare April 1, 2020 22:33
Copy link
Copy Markdown
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR @JulianoGTZ! This is looking good, but requires some changes.

Also, in regards to components, file names should be PascalCased and in regards to the folder called elements, rename it to components.

Thanks again for contributing!

Comment thread app/javascript/listings/elements/clearQueryButton.jsx Outdated
Comment thread app/javascript/listings/elements/messageModal.jsx Outdated
Comment thread app/javascript/listings/elements/modalBg.jsx Outdated
Comment thread app/javascript/listings/listings.jsx Outdated
Comment thread app/javascript/listings/listings.jsx Outdated
Comment thread app/javascript/listings/elements/modalBg.jsx Outdated
@JulianoGTZ
Copy link
Copy Markdown
Contributor Author

JulianoGTZ commented Apr 2, 2020

Hi @nickytonline. Thanks a lot for the Code Review. I'm going to work on this.

Do you have any tips to separate the responsibilities on the componentWillMount?

First, I thought about hooks, but for that I would need to update the version of the Preact.

@nickytonline
Copy link
Copy Markdown
Contributor

Hi @nickytonline. Thanks a lot for the Code Review. I'm going to work on this.

Do you have any tips to separate the responsibilities on the componentWillMount?

First, I thought about hooks, but for that, I would need to update the version of the Preact.

@JulianoGTZ, we cannot use hooks as you mentioned because it requires Preact X. I have a PR open for it, #5639, but it's blocked by the testing tools we use. We're looking into when we can schedule this upgrade as it's a little more involved than just upgrading Preact.

In regards to componentWillMount, use your judgement if you think things should be pulled out into a function or method or some other type of refactor. The one thing I would remove is this line: const t = this; and just replace all the ts with this.

@JulianoGTZ JulianoGTZ force-pushed the julianogtz/fixing-codeclimate-issue-on-listings branch from 1f859e0 to aa7e0a2 Compare May 2, 2020 00:09
@nickytonline
Copy link
Copy Markdown
Contributor

I didn’t forget about this @JulianoGTZ . I’ll be getting to this this week.

Copy link
Copy Markdown
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

@JulianoGTZ, an easy change to make, but one of the modal props is spelt incorrectly. You might also need to update some snapshots. To do that, run yarn test -u.

Comment thread app/javascript/listings/components/MessageModal.jsx Outdated
Comment thread app/javascript/listings/components/MessageModal.jsx Outdated
Comment thread app/javascript/listings/components/MessageModal.jsx Outdated
Comment thread app/javascript/listings/components/MessageModal.jsx Outdated
Comment thread app/javascript/listings/components/MessageModal.jsx Outdated
Comment thread app/javascript/listings/components/MessageModal.jsx Outdated
Comment thread app/javascript/listings/components/Modal.jsx Outdated
Comment thread app/javascript/listings/__tests__/MessageModal.test.jsx Outdated
@pr-triage pr-triage Bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels May 11, 2020
Copy link
Copy Markdown
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

There was a regression created when the listings page got split into smaller components. I've suggested a fix.

Comment thread app/javascript/listings/components/CategoryLinks.jsx Outdated
@pr-triage pr-triage Bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels May 11, 2020
@JulianoGTZ JulianoGTZ requested a review from nickytonline May 11, 2020 17:19
Copy link
Copy Markdown
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@pr-triage pr-triage Bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels May 11, 2020
@nickytonline nickytonline merged commit 18531f3 into forem:master May 11, 2020
@nickytonline
Copy link
Copy Markdown
Contributor

Thanks so much @JulianoGTZ for contributing to the DEV codebase and thanks for being patient during the PR review. 👏

@pr-triage pr-triage Bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: merged bot applied label for PR's that are merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants