Skip to content
This repository was archived by the owner on Aug 7, 2020. It is now read-only.

Conversation

rfougier
Copy link
Contributor

@rfougier rfougier commented May 17, 2018

/#!/oui-angular/modal-on-boarding @ ovh_ui_kit_documentation-feature_oui_modal_on_boarding

Linked to
ovh/ovh-ui-kit#208
ovh-ux/ovh-ui-kit-documentation#104

Copy link
Contributor

@AxelPeter AxelPeter left a comment

Choose a reason for hiding this comment

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

Caution on DOM manipulations outside digest loop/$timeout, we could have surprises in some cases. Apart from that, it's OK :)


| Attribute | Type | Binding | One-time Binding | Values | Default | Description |
| ---- | ---- | ---- | ---- | ---- | ---- | ---- |
| `on-dismiss` | function | &? | | | | dismiss callback |
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) For functions, you could set the binding to & directly.
If undefined, it will call angular.noop() instead.

this._isTouch = "ontouchstart" in window || navigator.msMaxTouchPoints > 0;
this._client = { x: 0, y: 0 };

this.$document.on("keydown", evt => this._triggerKeyHandler(evt));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be in $postLink

}

$postLink () {
this.panels = this.$element[0].querySelectorAll("oui-modal-on-boarding-panel");
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is DOM manipulations in your $postLink, we should wrap all with

this.$timeout(() => {
    [...]
});

this._isSliding = true;
const nextIsIllustrated = this._isIllustratedPanel(nextPanelIndex);

$nextPanel.addClass(direction);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as $postLink.
This should be in your $timeout below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DOM is sure all loaded at this time.
I don't think is required to do that !

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I think it's not necessary. $timeout is useful when we make DOM manipulation in $postLink because we're not sure that DOM has been inserted yet. Here, the DOM is clearly ready.

</div>
<div class="oui-modal-on-boarding__footer">
<div class="oui-modal-on-boarding__indicators"
data-ng-if="$ctrl.panels.length > 1">
Copy link
Contributor

Choose a reason for hiding this comment

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

No need of prefixing in our template

@AxelPeter
Copy link
Contributor

@rfougier Can you link all your PRs in the description ? Thanks :)

@antleblanc
Copy link
Contributor

@AxelPeter @rfougier You might consider using a .github/PULL_REQUEST_TEMPLATE.md file to handle that case?

@rfougier rfougier force-pushed the feature/oui-on-boarding branch 2 times, most recently from 00ff39b to 8de220a Compare May 23, 2018 15:53
keyCode: 27
});

// expect(dismissSpy).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't test anything in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i forgot to go back on it...

this._isSliding = true;
const nextIsIllustrated = this._isIllustratedPanel(nextPanelIndex);

$nextPanel.addClass(direction);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I think it's not necessary. $timeout is useful when we make DOM manipulation in $postLink because we're not sure that DOM has been inserted yet. Here, the DOM is clearly ready.

@rfougier rfougier force-pushed the feature/oui-on-boarding branch from 8de220a to c2461f5 Compare May 29, 2018 15:43
@whispyy whispyy changed the base branch from master to develop May 29, 2018 19:30
@rfougier rfougier force-pushed the feature/oui-on-boarding branch 3 times, most recently from 144046d to 4dbd58b Compare May 31, 2018 19:52
@AxelPeter AxelPeter force-pushed the feature/oui-on-boarding branch from 4dbd58b to 1b38be4 Compare June 11, 2018 11:32
@AxelPeter AxelPeter force-pushed the feature/oui-on-boarding branch from 1b38be4 to a27a89f Compare June 12, 2018 08:29
@AxelPeter AxelPeter changed the title feat(oui-modal-on-boarding): add new modal on Boarding component feat(oui-slideshow): add new slideshow component Jun 12, 2018
@AxelPeter AxelPeter merged commit 8f2e5f7 into develop Jun 12, 2018
@AxelPeter AxelPeter deleted the feature/oui-on-boarding branch June 12, 2018 09:07
neolitec pushed a commit that referenced this pull request Jun 21, 2018
* feat(oui-modal-on-boarding): add oui-modal-on-boarding feature

* chore: rename component to oui-slideshow

* chore(oui-slideshow): add onboarding theme
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Development

Successfully merging this pull request may close these issues.

5 participants