Skip to content

Conversation

@vtymtsiv
Copy link

@vtymtsiv vtymtsiv commented Sep 18, 2018

  • Add Service creation tutorial
  • Add Scripting tutorial

Copy link
Contributor

@OlegLazaryev OlegLazaryev left a comment

Choose a reason for hiding this comment

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

Reviewed.
Let's discuss.


});

subscribeHighlightingElement(step1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is duplicated several times. What in this case makes it hardly maintainable. I have some ideas how to generalize it. Let's discuss in-person.

}
});

var step1 = tour.addStep('services-tab', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have an idea of how to generalize the creation of steps. I think we can introduce a kind of a factory method to do that. Let's discuss in-person.

Copy link
Contributor

@OlegLazaryev OlegLazaryev left a comment

Choose a reason for hiding this comment

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

@volodymyrtymtsiv reviewed.
Let's discuss.

var selector = getSelector(step);

step.on('show', function () {
angular.element(selector).addClass('highlighted-element');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use jQuery selector instead of Angular.

angular.element(selector).addClass('highlighted-element');
});
step.on('before-hide', function () {
angular.element(selector).removeClass('highlighted-element')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use jQuery selector instead of Angular.

@OlegLazaryev
Copy link
Contributor

A general notice which is looking like a bug:

8ff3eef6_2018 25 09

Copy link
Contributor

@OlegLazaryev OlegLazaryev left a comment

Choose a reason for hiding this comment

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

@volodymyrtymtsiv
Left you some more comments.
I think it is going to be the last round of review.
We will improve the remaining technical debt in the next related PRs.
Let me now when finished.

@OlegLazaryev
Copy link
Contributor

@wjgilmore please review.

@wjgilmore
Copy link
Contributor

Thank you will review this week.

@vtymtsiv vtymtsiv removed their assignment Feb 10, 2020
@yaroslavmo yaroslavmo closed this May 1, 2020
@yaroslavmo yaroslavmo reopened this May 18, 2020
Copy link
Contributor

@wjgilmore wjgilmore left a comment

Choose a reason for hiding this comment

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

@volodymyrpoli do not merge this change. We now do this in Intercom.

Base automatically changed from develop to master June 16, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants