feat: add tour for launch #337

hasanagh · 2020-10-06T08:02:03Z

adds tour

close #323

adds tour close #323

juancarlosfarah · 2020-10-06T09:23:05Z

@hasanagh, please rebase w.r.t master.

pyphilia

Thank you for your PR! It was more complicated than I thought. I let you some comments, and here some general comment:

Do you think having a different components for each tour would be possible? At some point your Tour component will have to be expanded with a lot more tours and having different components (or something else) for each might help maintaining them.
I used the convention to name the selectors depending on the usage (id, class, name, etc). For example I would use VISIT_SPACE_BUTTON_CLASS instead of VISIT_SPACE_BUTTON (it mostly helps for tests)

pyphilia · 2020-10-07T09:04:58Z

public/app/config/channels.js

@@ -71,4 +71,5 @@ module.exports = {
  GET_SPACE_IN_CLASSROOM_CHANNEL: 'classroom:space:get',
  LOAD_SPACE_IN_CLASSROOM_CHANNEL: 'classroom:space:load',
  GET_SPACE_TO_LOAD_IN_CLASSROOM_CHANNEL: 'classroom:space:load:get-space',
+  TOUR_COMPLETED_CHANNEL: 'tour:complete',


I think COMPLETE_TOUR_CHANNEL would be better.

m

Thank you for your PR! It was more complicated than I thought. I let you some comments, and here some general comment:

Do you think having a different components for each tour would be possible? At some point your Tour component will have to be expanded with a lot more tours and having different components (or something else) for each might help maintaining them.

I used the convention to name the selectors depending on the usage (id, class, name, etc). For example I would use VISIT_SPACE_BUTTON_CLASS instead of VISIT_SPACE_BUTTON (it mostly helps for tests)

Hey Kim! Thanks!

Ok so for using a tour component for each tour: this was the approach I was using first. Then realized that it would make the process of switching between tours much more complex. Here we are managing one reducer for all the actions related to this tour and the tours expand for more than one component. (i.e if there is a tour for example only for the space screen without having to jump between screens then maybe we could consider separating tours). Also with the current structuring, any new tour (which I assume won't be many) we just need simply add a steps array and handle the callbacks. All managed in one space. of course, I plan maybe on expanding that architecture to a more refactored one especially in the callback where it looks a bit complicated maybe in future PRs and we can utilize it in other projects.

I will fix that

src/components/common/Tour.js

pyphilia · 2020-10-07T09:13:32Z

src/components/common/Tour.js

+            tour: tours.VISIT_SPACE_TOUR,
+            steps: VISIT_SPACE_TOUR_STEPS,
+          }),
+        750


Factor out this in constants.js

pyphilia · 2020-10-07T09:19:40Z

src/components/common/Tour.js

+                dispatchStartTour();
+                break;
+              case 2:
+                replace('/space/owozgj');


It could be worth to factor it out.

src/config/tours.js

juancarlosfarah · 2020-10-07T09:54:20Z

@hasanagh, don't forget to rebase.

juancarlosfarah

Very nice! Some changes requested. Also, please refactor to adapt to @pyphilia's latest PRs.

juancarlosfarah · 2020-10-14T13:46:52Z

src/actions/tour.js

+  STOP_TOUR,
+} from '../types/tour';
+
+const nextStepTour = payload => dispatch =>


Can we keep these as verbs? E.g. goToNextStep and goToPrevStep. Also, why is it NEXT_OR_PREV? For clarity we might want to have two separate actions.

juancarlosfarah · 2020-10-14T13:47:06Z

src/actions/tour.js

+    payload,
+  });
+
+const navigateStopTour = payload => dispatch =>


What is navigateStopTour?

This is an action I refactored out because when we have multi-page tours we need to stop the tour and then start it again.

Let's change it then to some other more descriptive name. Like stopTourAndNavigate or goToNextTour. Any proposals, @hasanagh, @pyphilia ?

Yes, I will go for stopTourAndNavigate more descriptive I think.

src/components/common/Tour.js

juancarlosfarah · 2020-10-14T13:49:08Z

src/components/common/Tour.js

+          },
+          options: {
+            primaryColor: THEME_COLORS[USER_MODES.TEACHER],
+            zIndex: 10000,


Can we factor this out?

juancarlosfarah · 2020-10-14T13:49:21Z

src/components/common/Tour.js

+        }}
+        callback={callback}
+        locale={{
+          last: 'End tour',


Can we use t for these?

src/config/tours.js

juancarlosfarah · 2020-10-14T13:58:09Z

src/config/tours.js

+  },
+  {
+    target: `.${SETTINGS_ACTIONS_CLASS}`,
+    content: 'Here you can enable tracking of actions within spaces',


Track analytics by activating this switch.

juancarlosfarah · 2020-10-14T13:58:58Z

src/types/tour.js

@@ -0,0 +1,7 @@
+export const NEXT_OR_PREV_TOUR = 'NEXT_OR_PREV_TOUR';
+export const NAVIGATE_STOP_TOUR = 'NAVIGATE_STOP_TOUR';


Not sure what this means.

See comment above.

juancarlosfarah · 2020-10-14T13:59:08Z

src/types/tour.js

@@ -0,0 +1,7 @@
+export const NEXT_OR_PREV_TOUR = 'NEXT_OR_PREV_TOUR';


Maybe split in two for legibility.

Also, use verbs.

juancarlosfarah · 2020-10-14T13:59:56Z

src/utils/elements.js

@@ -0,0 +1,11 @@
+const waitForElement = async ({ selector, time = 500 }) => {


Can this cause an infinite loop?

Yes, in case the selector never showed up in the dom. I assumed in this case, anyway, there would have been a problem. But if you want maybe we could limit the number of retries?

Maybe you could have a timeout or a number of retries, otherwise show a toastr error notifying that the tour couldn't be launched.

…o 323/implement-a-tour-guide

juancarlosfarah

Very nice! We're almost there. Just some minor changes and comments.

juancarlosfarah · 2020-10-16T16:15:16Z

src/actions/tour.js

+    payload,
+  });
+
+const navigateStopTour = payload => dispatch =>


Let's change it then to some other more descriptive name. Like stopTourAndNavigate or goToNextTour. Any proposals, @hasanagh, @pyphilia ?

juancarlosfarah · 2020-10-16T16:17:08Z

src/components/common/Tour.js

+                  selector: `.${VISIT_SPACE_INPUT_CLASS}`,
+                });
+                document.getElementById(DRAWER_BUTTON_ID).click();
+                setTimeout(() => dispatchStartTour(), TOUR_DELAY_500);


Still, I would create a function in this class called handleStartTour and put this line inside.

juancarlosfarah · 2020-10-16T16:18:16Z

src/config/tours.js

+    disableBeacon: true,
+  },
+  {
+    target: `.${SPACE_SCREEN_CLASS}`,


I think it's redundant with the previous step. Let's remove it.

juancarlosfarah · 2020-10-16T16:19:06Z

src/config/tours.js

+    target: `.${VISIT_SPACE_INPUT_CLASS}`,
+    title: 'Visiting a Space',
+    content:
+      "You can visit a space using its URL or ID. For example, let's have a look at space https://graasp.eu/s/owozgj or simply owozgj.",


You can use the factored out value of the sample space here too.

Sorry I did not understand your proposal. What do you mean?

content: `You can... have a look at space https://...${TOUR_SPACE}... or ${TOUR_SPACE}`

src/components/common/Tour.js

juancarlosfarah · 2020-10-16T16:21:12Z

src/types/tour.js

@@ -0,0 +1,7 @@
+export const NEXT_OR_PREV_TOUR = 'NEXT_OR_PREV_TOUR';
+export const NAVIGATE_STOP_TOUR = 'NAVIGATE_STOP_TOUR';


See comment above.

juancarlosfarah

Just one very minor change and a question about whether you are keeping that prev condition or removing it. You discussed this with @pyphilia.

juancarlosfarah · 2020-10-20T07:57:59Z

src/components/common/Tour.js

+        }}
+        callback={callback}
+        locale={{
+          last: t('End tour'),


"Tour" capitalised.

hasanagh · 2020-10-20T09:32:51Z

Just one very minor change and a question about whether you are keeping that prev condition or removing it. You discussed this with @pyphilia.

For me, I think we should keep it to ensure the tour launch dispatch won't be fired twice. But I am ok with removing it as it doesn't cause any issues.

juancarlosfarah · 2020-10-24T16:00:37Z

What happened with this PR? Can we go ahead and merge it? Would be great to release it next week.

hasanagh · 2020-10-24T17:02:17Z

What happened with this PR? Can we go ahead and merge it? Would be great to release it next week.

Done with it. We can merge!

pyphilia

I added minor comments, the most important part is the tests where the syntax changed a bit.

pyphilia · 2020-10-26T15:46:52Z

src/components/common/Tour.js

+            }
+            break;
+          default:
+            dispatchCompleteTour(tours.VISIT_SPACE_TOUR);


Do we really want to dispatch VISIT_SPACE_TOUR by default?

pyphilia · 2020-10-26T15:50:37Z

src/utils/elements.js

@@ -0,0 +1,11 @@
+const waitForElement = async ({ selector, time = 500 }) => {


Maybe you could have a timeout or a number of retries, otherwise show a toastr error notifying that the tour couldn't be launched.

pyphilia · 2020-10-26T15:53:02Z

test/utils.js

@@ -89,6 +90,7 @@ export const menuGoToPhase = async (client, nb) => {

 export const menuGoToSettings = async (client) => {
  await menuGoTo(client, SETTINGS_MENU_ITEM_ID, SETTINGS_MAIN_ID);
+  await client.click(TOUR_END_SELECTOR);


You will have to change it for: (because of the dependencies update)

const tourEndButton = await client.$(TOUR_END_SELECTOR); await tourEndButton.click()

pyphilia · 2020-10-26T15:53:24Z

test/utils.js

 ) => {
  const input = await client.$(`#${LOGIN_USERNAME_INPUT_ID}`);
  await input.setValue(name);
  await client.pause(INPUT_TYPE_PAUSE);
  const button = await client.$(`#${LOGIN_BUTTON_ID}`);
  await button.click();
  await client.pause(LOGIN_PAUSE);
-
+  if (closeTour) {
+    await client.click(TOUR_END_SELECTOR);


Same as above.

…o 323/implement-a-tour-guide

feat: add tour for launch

7610b53

adds tour close #323

hasanagh requested review from juancarlosfarah and pyphilia October 6, 2020 08:02

pyphilia requested changes Oct 7, 2020

View reviewed changes

hasanagh force-pushed the 323/implement-a-tour-guide branch from bfaa4fa to c565bf7 Compare October 7, 2020 22:03

juancarlosfarah requested changes Oct 14, 2020

View reviewed changes

Merge branch 'master' of https://github.com/graasp/graasp-desktop int…

1ce1c7e

…o 323/implement-a-tour-guide

hasanagh force-pushed the 323/implement-a-tour-guide branch from c565bf7 to 1ce1c7e Compare October 15, 2020 18:27

juancarlosfarah requested changes Oct 16, 2020

View reviewed changes

hasanagh force-pushed the 323/implement-a-tour-guide branch from 940e452 to d089623 Compare October 18, 2020 12:15

juancarlosfarah requested changes Oct 20, 2020

View reviewed changes

src/components/common/Tour.js Outdated

}}

callback={callback}

locale={{

last: t('End tour'),

Copy link

Member

juancarlosfarah Oct 20, 2020

Choose a reason for hiding this comment

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

"Tour" capitalised.

hasanagh force-pushed the 323/implement-a-tour-guide branch from d089623 to 1be08a1 Compare October 24, 2020 17:02

pyphilia requested changes Oct 26, 2020

View reviewed changes

Merge branch 'master' of https://github.com/graasp/graasp-desktop int…

8a388ba

…o 323/implement-a-tour-guide

hasanagh force-pushed the 323/implement-a-tour-guide branch from 1be08a1 to 8a388ba Compare October 26, 2020 18:29

pyphilia merged commit f188f27 into master Oct 27, 2020

pyphilia deleted the 323/implement-a-tour-guide branch October 27, 2020 08:47

		@@ -0,0 +1,7 @@
		export const NEXT_OR_PREV_TOUR = 'NEXT_OR_PREV_TOUR';
		export const NAVIGATE_STOP_TOUR = 'NAVIGATE_STOP_TOUR';

		@@ -0,0 +1,11 @@
		const waitForElement = async ({ selector, time = 500 }) => {

feat: add tour for launch #337

feat: add tour for launch #337

Conversation

hasanagh commented Oct 6, 2020

juancarlosfarah commented Oct 6, 2020

pyphilia left a 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

juancarlosfarah commented Oct 7, 2020

juancarlosfarah left a 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

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

juancarlosfarah left a 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

juancarlosfarah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasanagh commented Oct 20, 2020

juancarlosfarah commented Oct 24, 2020

hasanagh commented Oct 24, 2020

pyphilia left a 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