Fredhase/codeclimate refactor onboarding js 3739 #5767

fredhase · 2020-01-27T16:30:15Z

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

Description

Made a function for repetitive blocks in render function for app/javascript/onborading/components/ClosingSlide.tsx

Added to documentation?

docs.dev.to
readme
no documentation needed

nickytonline

Thanks so much for the PR @fredhase! I've requested a few changes and also some suggestions.

nickytonline · 2020-01-27T20:09:09Z

app/assets/javascripts/initializers/initializeDrawerSliders.js

+    drawerSliders.forEach(drawerSliders => {
+      const element = document.getElementById(drawerSlider.selector);
+      if (element) {
+        element.onclick = function() {


Instead of having 4 click events, you can add one click event (via addEventListener) on the element with the ID on-page-nav-controls. From there you can compare the Event.target.id with the drawerSliders. To avoid looping through the array, I'd suggest making it an object instead, e.g.

const drawerSliders = { 'sidebar-bg-left': { swipeState: 'middle', side: 'left', view: 'outOfView' }, 'sidebar-bg-right': { swipeState: 'middle', side: 'right', view: 'outOfView', }, 'on-page-nav-butt-left': { swipeState: 'left', side: 'left', view: 'intoView', }, 'on-page-nav-butt-right': { swipeState: 'right', side: 'right', view: 'intoView', }, };

I get the ideia of the addEventListener, but I don't see where/how exactly to implement this. 🤔

I'll post an example in a bit. Just in a meeting.

I've made some testing here. I'll post it.
Using the array, it works fine:

`if (document.getElementById('on-page-nav-controls')) {
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' },
];

drawerSliders.forEach((drawerSlider) => { const element = document.getElementById(drawerSlider.selector); if (element) { element.onclick = function() { swipeState = drawerSlider.swipeState; slideSidebar(drawerSlider.side, drawerSlider.view); }; } }); body.addEventListener('click', () => { slideSidebar('right', 'outOfView'); slideSidebar('left', 'outOfView'); }); listenForNarrowMenuClick();

}
}`

But using as an object, the side bar doesn't close when I click elsewhere:

` const element = document.getElementById('on-page-nav-controls')

if (element) {
const drawerSliders = {
'sidebar-bg-left': { swipeState: 'middle', side: 'left', view: 'outOfView' },
'sidebar-bg-right': { swipeState: 'middle', side: 'right', view: 'outOfView' },
'on-page-nav-butt-left': { swipeState: 'left', side: 'left', view: 'intoView' },
'on-page-nav-butt-right': { swipeState: 'right', side: 'right', view: 'intoView' },
};

element.addEventListener('click', (e) => { const drawerSlider = drawerSliders[e.target.id] swipeState = drawerSlider.swipeState; slideSidebar(drawerSlider.side, drawerSlider.view); }); body.addEventListener('click', () => { slideSidebar('right', 'outOfView'); slideSidebar('left', 'outOfView'); }); listenForNarrowMenuClick();

}
}`

Any suggestions or observations?
Also, meanwhile I'm working on the others changes

This is what I meant if using an object for drawer sliders, but the same could be done with the array. The only difference with the array is you'd need to loop through or do a .find to, get the right slider when comparing it to the clicked element's className. My assumption for this is there is only that one class name, which may not be correct. If there is more than one class name, looping through the array may make more sense to find the right drawer slider.

const navControls = document.getElementById("on-page-nav-controls"); const drawerSliders = { 'sidebar-bg-left': { swipeState: 'middle', side: 'left', view: 'outOfView' }, 'sidebar-bg-right': {. swipeState: 'middle', side: 'right', view: 'outOfView', }, 'on-page-nav-butt-left': { swipeState: 'left', side: 'left', view: 'intoView', }, 'on-page-nav-butt-right': { swipeState: 'right', side: 'right', view: 'intoView', }, }; navControls.addEventListener('click', e => { // e.target.className would be for example 'sidebar-bg-left' const drawerSlider = drawerSliders[e.target.className] if (!drawerSlider) { // a drawer slider was not clicked. return; } const { side, view } = drawerSlider; slideSidebar(side, view); // This appears to be a global as it is not set in this file swipeState = drawerSlider.swipeState; });

nickytonline · 2020-01-27T20:10:03Z

app/assets/javascripts/initializers/initializeDrawerSliders.js

      }
-    }
+    });
+
    InstantClick.on('change', function() {
      document.getElementsByTagName('body')[0].classList.remove('modal-open');


Since we're doing cleanup.

Suggested change

document.getElementsByTagName('body')[0].classList.remove('modal-open');

document.body.classList.remove('modal-open');

nickytonline · 2020-01-27T20:12:42Z

app/assets/javascripts/initializers/initializeDrawerSliders.js

-  var narrowFeedButt = document.getElementById("narrow-feed-butt");
-  for (var i = 0; i < navLinks.length; i++) {
-    document.getElementById("narrow-nav-menu").classList.remove("showing");
+  var navLinks = document.getElementsByClassName('narrow-nav-menu');


If you have time, it'd be nice to migrate var to const and let where necessary. I use const as a default, but there are no strong opinions on this in the codebase.

nickytonline · 2020-01-27T20:14:59Z

app/assets/javascripts/initializers/initializeDrawerSliders.js

-    narrowFeedButt.onclick = function(){
-      document.getElementById("narrow-nav-menu").classList.add("showing");
-    }
+    narrowFeedButt.onclick = function() {


This exists in other parts of the codebase as well, but we should be defaulting to using addEventListener.

Suggested change

narrowFeedButt.onclick = function() {

narrowFeedButt.addEventListener('click', function() {

nickytonline · 2020-01-27T20:20:28Z

app/javascript/onboarding/utilities.js

@@ -6,3 +6,16 @@ export const jsonToForm = data => {

 export const getContentOfToken = token =>


I don't believe we use this anywhere else but for the CSRF token, so consider just naming this getCsrfToken and omit the token parameter and hard-code csrf-token in the function. If in the future we need to get the content of different meta tags, then we should consider promoting this to a more generic function like you currently have.

Also, consider adding some unit tests to these functions as well. Happy to help on this front.

nickytonline · 2020-01-27T20:22:50Z

app/javascript/onboarding/components/ClosingSlide.jsx

+
+    return(
+      properties.map((element, index) => 
+          <a key={ index } href={ element.href } data-no-instant={(index === 0) ? true : null }>


+1 for remembering the key 😎

nickytonline · 2020-01-27T20:28:35Z

app/assets/javascripts/initializers/initializeSwipeGestures.js

-      touchcancel:function(e){nm=false}
-    };
-    for(var a in touch){d.addEventListener(a,touch[a],false);}
+        var nm = true;


I see prettier in action here, and it looks like this code was probably pulled from somewhere minified. It looks like it's just registering events, but if you have more insight, consider adding a comment about this.

fredhase · 2020-01-28T13:23:13Z

hey @nickytonline
Thanks for the reply and review :)
I'll see what I can do, but it's very nice, not only for the project, but also for me, as an intern and begginer, to grow and learn more. I'll try my best to give the best result

nickytonline · 2020-01-28T13:30:26Z

hey @nickytonline
Thanks for the reply and review :)
I'll see what I can do, but it's very nice, not only for the project, but also for me, as an intern and begginer, to grow and learn more. I'll try my best to give the best result

Cool cool @fredhase . As mentioned, if you need some help with any of this, don’t hesitate to @ me.

fredhase · 2020-02-12T16:55:45Z

Closing this PR for some branche mistakes that caused conflict

hasegawa-f added 7 commits January 17, 2020 14:35

made function for repetitive blocks

43a8a19

made and renamed function for repetitive blocks

bfbb266

Initializers JS: refactior repetitive blocks to function

a56cd96

Initializers JS: refactior repetitive blocks to function

d61e587

Initializers JS: refactior repetitive blocks to function

3b9d76e

Onboarding JS: Refactor repetitive rendered blocks to function

f31a920

Onboarding JS: Refactor repetitive rendered blocks to function

ec063a6

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 27, 2020

rhymes requested review from jacobherrington and nickytonline January 27, 2020 17:00

nickytonline suggested changes Jan 27, 2020

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 Jan 27, 2020

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

fredhase closed this Feb 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fredhase/codeclimate refactor onboarding js 3739 #5767

Fredhase/codeclimate refactor onboarding js 3739 #5767

fredhase commented Jan 27, 2020

nickytonline left a comment

nickytonline Jan 27, 2020 •

edited

Loading

fredhase Jan 28, 2020

nickytonline Jan 28, 2020

fredhase Jan 29, 2020

nickytonline Jan 29, 2020

nickytonline Jan 27, 2020

nickytonline Jan 27, 2020

nickytonline Jan 27, 2020

nickytonline Jan 27, 2020

nickytonline Jan 27, 2020 •

edited

Loading

nickytonline Jan 27, 2020

nickytonline Jan 27, 2020

fredhase commented Jan 28, 2020

nickytonline commented Jan 28, 2020

fredhase commented Feb 12, 2020

	document.getElementsByTagName('body')[0].classList.remove('modal-open');
	document.body.classList.remove('modal-open');

	narrowFeedButt.onclick = function() {
	narrowFeedButt.addEventListener('click', function() {

		@@ -6,3 +6,16 @@ export const jsonToForm = data => {

		export const getContentOfToken = token =>

Fredhase/codeclimate refactor onboarding js 3739 #5767

Fredhase/codeclimate refactor onboarding js 3739 #5767

Conversation

fredhase commented Jan 27, 2020

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

Description

Added to documentation?

nickytonline left a comment

Choose a reason for hiding this comment

nickytonline Jan 27, 2020 • edited Loading

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

nickytonline Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredhase commented Jan 28, 2020

nickytonline commented Jan 28, 2020

fredhase commented Feb 12, 2020

nickytonline Jan 27, 2020 •

edited

Loading

nickytonline Jan 27, 2020 •

edited

Loading