-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fredhase/codeclimate refactor onboarding initializers 3739 #5809
Fredhase/codeclimate refactor onboarding initializers 3739 #5809
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build is failing,
And also, refactor without unit tests can be dangerous. I suggest writing some in a separate PR prior to this one.
const drawerSliders = [ | ||
{ selector: 'sidebar-bg-left', swipeState: 'middle', side: 'left', view: 'outOfView' }, | ||
{ selector: 'sidebar-bg-right', swipeState: 'middle', side: 'right', view: 'outOfView' }, | ||
{ selector: 'on-page-nav-butt-left', swipeState: 'left', side: 'left', view: 'intoView' }, | ||
{ selector: 'on-page-nav-butt-right', swipeState: 'right', side: 'right', view: 'intoView' }, | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be kept outside the function.
} | ||
} | ||
InstantClick.on('change', function() { | ||
document.getElementsByTagName('body')[0].classList.remove('modal-open'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This classList.remove('modal-open')
seems to be missed in the new code.
} else { | ||
swipeState = 'middle'; | ||
slideSidebar('left', 'outOfView'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can avoid using else
at all
if (!document.getElementById('on-page-nav-controls')) {
return;
}
if (swipeState == 'middle') {
swipeState = 'right';
return slideSidebar('right', 'intoView');
}
swipeState = 'middle';
slideSidebar('left', 'outOfView');
@@ -125,42 +138,10 @@ class ClosingSlide extends Component { | |||
</div> | |||
); | |||
} | |||
return ( | |||
return( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a blank space
} | ||
|
||
whatsNext() { | ||
const properties = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, rename this to something like whatsNextLinks
,
or just return the array (see next comment)
return( | ||
properties.map((element, index) => | ||
<a key={ index } href={ element.href } data-no-instant={(index === 0) ? true : null }> | ||
{ element.text } | ||
<p className="whatnext-emoji"> | ||
<span role="img" aria-label="tada"> | ||
{ element.emoji } | ||
</span> | ||
</p> | ||
</a> | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't add much to have a method return a small piece of component here.
As a suggestion:
- make
whatsNext()
method return just the array of objects. .map
it inrender()
method
@paulodiovani |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickytonline |
No worries @fredhase. And no pressure to get this done. If you need some assistance with branches and git, happy to help. This post from one of our community members may be of help to you, https://dev.to/unseenwizzard/learn-git-concepts-not-commands-4gjc |
Closing this PR for some branche mistakes that caused conflict |
What type of PR is this? (check all applicable)
Description
Refactoring codes in app/assets/javascripts/initializers (DrawerSliders and SwipeGestures). Some minor refactoring and some repetitive blocks are now a function.
In app/javascript/onboarding/components just made minor refactoring.
Added to documentation?