Refactor/lint onboarding slides #4240

daningenthron · 2019-10-05T01:55:59Z

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

[ X ] Refactor
Feature
Bug Fix
Documentation Update

Description

From #2470 - cleans up ESlint errors that weren't passing Husky. For the most part these were small edits (snake case to camel case etc), although two changes may need some attention:

EmailListTermsConditionsForm.jsx contained a dangerouslySetInnerHTML warning that wouldn't pass; this has been bypassed by disabling the ESLint rule for this line. I think a better fix could be adding the DOMPurify dependency and sanitizing the HTML here and adding via innerHTML vs dangerouslySetInnerHTML, but those changes didn't seem in line with this PR.
In SlideContent.jsx I set up a blank (non-validating) shape for the style PropType. Not sure if this is best practice, but it seemed to make sense as far as trying to appease ESLint without defining the CSS attributes (and their types) ahead of time.

Added to documentation?

docs.dev.to
readme
[ X ] no documentation needed

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

Hand pulling lint out of a lint trap in a single super-thick sheet

CLAassistant · 2019-10-05T01:56:23Z

All committers have signed the CLA.

jacobherrington

Thanks for making a PR @daningenthron! 🎉

I made some of these changes in #4254, but I will rebase and take your changes in my branch after this has been merged.

Happy Hacktoberfest! 🎃

maestromac · 2019-10-07T19:00:32Z

app/javascript/onboarding/components/EmailListTermsConditionsForm.jsx

+      checkedCodeOfConduct: false,
+      checkedTermsAndConditions: false,
+      emailMembershipNewsletter: true,
+      emailDigestPeriodic: true,


Not that this will technically be breaking the onboard process, but it won't properly update user's preference. This is because the whole state is sent via fetch as param.

I still want to accept this and make changes to the controller to snake-case params but reverting this refactor could be more straight foward. Do you have any thought on this @jacobherrington ?

Firstly, we probably want a feature spec to validate that onboarding works, going forward. That's something I'm happy to tackle in another PR.

Secondly, I think we should revert these changes in this PR and handle that in a new PR; I believe there are quite a few ways to address this and I'm not sure the Rails community has settled on one. Having it in a separate PR will allow us to merge this one and discuss it at length.

It's definitely a little weird to see snake case in JavaScript, but it's also weird to see camel case in Ruby.

I found an answer to this on SO, but I'm not sure it's the best route.

jacobherrington

I'm going to dismiss my approval based on Mac's thoughts. We should probably approach this a bit differently.

jacobherrington · 2019-10-07T20:10:03Z

app/javascript/onboarding/components/EmailListTermsConditionsForm.jsx

+      checkedCodeOfConduct: false,
+      checkedTermsAndConditions: false,
+      emailMembershipNewsletter: true,
+      emailDigestPeriodic: true,


I think we should avoid making these snake_case to camelCase changes in this PR and tackle it in another PR based on @maestromac's feedback. Would you mind undoing this change? I'd love to merge in the other linting changes.

No problem at all, thanks for reviewing @jacobherrington! Resubmitting now.

jacobherrington

Could we go ahead and undo all of the camel case changes? I'm still seeing some data not make it through the onboarding slides.

jacobherrington · 2019-10-09T18:25:46Z

app/javascript/onboarding/components/SlideContent.jsx

@@ -12,17 +17,17 @@ const SlideContent = ({ imageSource, imageAlt, content, style = { textAlign: 'ce
    {content}
    <p>
      <strong>
-        <em>Let's get started...</em>
+        <em>Let`&apos;`s get started...</em>


We don't need the backticks here, if we leave them in, it shows up like this:

jacobherrington · 2019-10-09T18:28:05Z

app/javascript/onboarding/components/PersonalInfoForm.jsx

          location,
-          employer_name,
-          employment_title,
+          employerName,


This information doesn't seem to be making it through, this probably needs to go back to snake case as well.

… slide

daningenthron · 2019-10-12T20:44:36Z

Hi @jacobherrington, removed the additional camelCase changes, and the backticks as requested. PersonalInfoForm.jsx has merged changes that have gone in since the original commit, with the variable setupFormTextField declared outside the class. ESLint was throwing a 'missing PropTypes' error on this function's params; I added a second PropTypes validator, but this appears to be the only JSX file in the repo with 2 separate validators. Would you recommend a different fix?

daningenthron · 2019-10-23T19:54:54Z

Hi @jacobherrington just wanted to check in one more time - should I go ahead and close this PR at this point?

jacobherrington · 2019-11-08T16:54:51Z

Hey @daningenthron it looks like there are some conflicts. If you rebase this branch, we can see what changes should be merged.

maestromac · 2019-12-16T20:37:00Z

Sorry for letting this sit @daningenthron . I'm going to resolve the merge conflict and do the necessary work to get this merged.

maestromac

LGTM!

Sorry that some of your lost work because the time this PR has been sitting 🙇

jacobherrington

LGTM this got lost in the shuffle a little bit, but we are good to merge these changes imo.

Thanks for making this PR @daningenthron ❤️

daningenthron · 2019-12-16T23:27:21Z

Thanks so much @maestromac & @jacobherrington! Wasn't sure about moving this forward bc of the time and the refactor - thanks for finishing it up!

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 5, 2019

rhymes mentioned this pull request Oct 6, 2019

Remove unused onboarding files #4254

Merged

7 tasks

weekly-digest bot mentioned this pull request Oct 7, 2019

Weekly Digest (30 September, 2019 - 7 October, 2019) #4273

Closed

jacobherrington approved these changes Oct 7, 2019

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 7, 2019

maestromac reviewed Oct 7, 2019

View reviewed changes

jacobherrington self-requested a review October 7, 2019 20:08

jacobherrington removed the PR: reviewed-approved bot applied label for PR's where reviewer approves changes label Oct 7, 2019

jacobherrington suggested changes Oct 7, 2019

View reviewed changes

pr-triage bot added the PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes label Oct 7, 2019

daningenthron force-pushed the refactor/lint-onboarding-slides branch from 16e1784 to 3d0a774 Compare October 9, 2019 00:56

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 Oct 9, 2019

jacobherrington suggested changes Oct 9, 2019

View reviewed changes

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 Oct 9, 2019

daningenthron added 5 commits October 12, 2019 15:36

Update onboarding slides per ESlint

f1f49fb

Update tests & Jest snapshots

be397c7

Reverting snake-to-camelcase changes and disabling linter rule in T&C…

3eb7290

… slide

Revert remaining snake > camel changes

76f2abc

Revert tests/snapshot to remove camel case

7d09618

daningenthron force-pushed the refactor/lint-onboarding-slides branch from 3d0a774 to 7d09618 Compare October 12, 2019 20:36

daningenthron requested a review from a team October 12, 2019 20:36

pr-triage bot removed the PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes label Oct 12, 2019

ghost requested a review from maestromac October 12, 2019 20:36

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 12, 2019

ghost removed their request for review October 12, 2019 20:36

weekly-digest bot mentioned this pull request Oct 14, 2019

Weekly Digest (7 October, 2019 - 14 October, 2019) #4406

Closed

daningenthron mentioned this pull request Oct 22, 2019

Eslint errors on *.jsx files #2470

Closed

maestromac added 2 commits December 16, 2019 15:32

Resolve merge commit

0ca57e9

Update snapshot

ed43dce

maestromac approved these changes Dec 16, 2019

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Dec 16, 2019

maestromac requested a review from jacobherrington December 16, 2019 21:51

jacobherrington approved these changes Dec 16, 2019

View reviewed changes

maestromac merged commit 12dae72 into forem:master Dec 16, 2019

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Dec 16, 2019

daningenthron deleted the refactor/lint-onboarding-slides branch December 16, 2019 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/lint onboarding slides #4240

Refactor/lint onboarding slides #4240

daningenthron commented Oct 5, 2019 •

edited

Loading

CLAassistant commented Oct 5, 2019 •

edited

Loading

jacobherrington left a comment

maestromac Oct 7, 2019

jacobherrington Oct 7, 2019

jacobherrington left a comment

jacobherrington Oct 7, 2019

daningenthron Oct 8, 2019

jacobherrington left a comment

jacobherrington Oct 9, 2019

jacobherrington Oct 9, 2019

daningenthron commented Oct 12, 2019

daningenthron commented Oct 23, 2019

jacobherrington commented Nov 8, 2019

maestromac commented Dec 16, 2019

maestromac left a comment

jacobherrington left a comment

daningenthron commented Dec 16, 2019

Refactor/lint onboarding slides #4240

Refactor/lint onboarding slides #4240

Conversation

daningenthron commented Oct 5, 2019 • edited Loading

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

Description

Added to documentation?

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

CLAassistant commented Oct 5, 2019 • edited Loading

jacobherrington left a comment

Choose a reason for hiding this comment

maestromac Oct 7, 2019

Choose a reason for hiding this comment

jacobherrington Oct 7, 2019

Choose a reason for hiding this comment

jacobherrington left a comment

Choose a reason for hiding this comment

jacobherrington Oct 7, 2019

Choose a reason for hiding this comment

daningenthron Oct 8, 2019

Choose a reason for hiding this comment

jacobherrington left a comment

Choose a reason for hiding this comment

jacobherrington Oct 9, 2019

Choose a reason for hiding this comment

jacobherrington Oct 9, 2019

Choose a reason for hiding this comment

daningenthron commented Oct 12, 2019

daningenthron commented Oct 23, 2019

jacobherrington commented Nov 8, 2019

maestromac commented Dec 16, 2019

maestromac left a comment

Choose a reason for hiding this comment

jacobherrington left a comment

Choose a reason for hiding this comment

daningenthron commented Dec 16, 2019

daningenthron commented Oct 5, 2019 •

edited

Loading

CLAassistant commented Oct 5, 2019 •

edited

Loading