Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor listings/listings.jsx #6992

Conversation

JulianoGTZ
Copy link
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

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
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!

app/javascript/listings/elements/clearQueryButton.jsx Outdated Show resolved Hide resolved
app/javascript/listings/elements/messageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/elements/modalBg.jsx Outdated Show resolved Hide resolved
app/javascript/listings/listings.jsx Outdated Show resolved Hide resolved
app/javascript/listings/listings.jsx Outdated Show resolved Hide resolved
app/javascript/listings/elements/modalBg.jsx Outdated Show resolved Hide resolved
@JulianoGTZ
Copy link
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
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
Contributor

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

Copy link
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.

app/javascript/listings/components/MessageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/components/MessageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/components/MessageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/components/MessageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/components/MessageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/components/MessageModal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/components/Modal.jsx Outdated Show resolved Hide resolved
app/javascript/listings/__tests__/MessageModal.test.jsx Outdated Show resolved Hide resolved
@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
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.

app/javascript/listings/components/CategoryLinks.jsx Outdated Show resolved Hide resolved
@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
Copy link
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
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.

None yet

4 participants