Skip to content

Conversation

Iucapad
Copy link
Contributor

@Iucapad Iucapad commented Nov 15, 2021

This commit adds the documentation related to
the scroller service. This service is used for
the webclient to handle clicks on links and there
were no documentation related until now.

@Iucapad Iucapad requested a review from SimonGenin November 15, 2021 12:40
@robodoo
Copy link
Collaborator

robodoo commented Nov 15, 2021

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, it's not a part of the scroller service, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not, so I don't know if and were I could document that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ged-odoo I removed it. Is it better now?

@Iucapad Iucapad force-pushed the 15.0-scroller-service-luvi branch 3 times, most recently from 976411f to 4d23086 Compare November 17, 2021 08:24
- Technical name: `scroller`
- Dependencies: none

Provides a way to handle scrolls to anchors in the webclient.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe clearer to explain what it does. Suggestion: Replace the 2 lines with:

Whenever the user clicks on an anchor in the web client, this service automatically scrolls
to the target (if appropriate).

Provides a way to handle scrolls to anchors in the webclient.
This service is used whenever a click occurs on the webclient.

The service adds an event listener to get `click`'s on the document. The service is then
Copy link
Contributor

Choose a reason for hiding this comment

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

use present tense: is then checking => checks


The service adds an event listener to get `click`'s on the document. The service is then
checking if the selector contained in its href attribute is valid to distinguish anchors and Odoo
actions (e.g. ``<a href="#target_element"></a>``). It returns nothing if it is not the case.
Copy link
Contributor

Choose a reason for hiding this comment

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

it does not returns nothing. it just does nothing.

checking if the selector contained in its href attribute is valid to distinguish anchors and Odoo
actions (e.g. ``<a href="#target_element"></a>``). It returns nothing if it is not the case.

An event ``SCROLLER:ANCHOR_LINK_CLICKED`` is triggered on the main application bus. That event contains a
Copy link
Contributor

Choose a reason for hiding this comment

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

If the click seems to be targeted at an element, then an event ...

An event ``SCROLLER:ANCHOR_LINK_CLICKED`` is triggered on the main application bus. That event contains a
custom event containing the `element` matching and its `id` to notify other parts that may need to handle
an anchor behavior themselves. The original event is also given as it might need to be prevented. Otherwise,
the ``scrollTo`` function is used to handle the scroll to the correct element directly in the service.
Copy link
Contributor

Choose a reason for hiding this comment

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

here and elsewhere: you can use single quote instead of double quote


An event ``SCROLLER:ANCHOR_LINK_CLICKED`` is triggered on the main application bus. That event contains a
custom event containing the `element` matching and its `id` to notify other parts that may need to handle
an anchor behavior themselves. The original event is also given as it might need to be prevented. Otherwise,
Copy link
Contributor

Choose a reason for hiding this comment

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

replace Otherwise with: `If the event is not prevented, then the user interface will scroll to the target element.

No need to explain that it uses the scrollto function. this is an internal implementation detail

@Iucapad Iucapad force-pushed the 15.0-scroller-service-luvi branch from 4d23086 to 31d299b Compare November 18, 2021 16:18
@ged-odoo ged-odoo requested a review from a team November 18, 2021 16:21
Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

@robodoo delegate+

@AntoineVDV
Copy link
Collaborator

@robodoo delegate=ged-odoo

This commit adds the documentation related to
the scroller service. This service is used for
the webclient to handle clicks on links and there
were no documentation related until now.
@Iucapad Iucapad force-pushed the 15.0-scroller-service-luvi branch from 31d299b to f809e7b Compare November 19, 2021 10:26
@Iucapad Iucapad requested a review from Feyensv November 19, 2021 10:39
Copy link
Collaborator

@Feyensv Feyensv left a comment

Choose a reason for hiding this comment

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

No need to rerequest a review if you only fixed a minor typo.
You can merge it or leave the r+ to ged, the delegate has effect even after a force-push anyway

@Iucapad
Copy link
Contributor Author

Iucapad commented Nov 19, 2021

@robodoo r+

@robodoo
Copy link
Collaborator

robodoo commented Nov 19, 2021

I'm sorry, @Iucapad. I must know your email before you can review PRs. Please contact an administrator.

@ged-odoo
Copy link
Contributor

robodoo r+

@robodoo robodoo closed this in 1f84bab Nov 19, 2021
@robodoo robodoo temporarily deployed to merge November 19, 2021 11:42 Inactive
@AntoineVDV
Copy link
Collaborator

AntoineVDV commented Nov 19, 2021

Looks like delegate= works after a force-push but delegate+ doesn't 🤨
Or only one person can be delegated the r+ at a time?

@fw-bot fw-bot deleted the 15.0-scroller-service-luvi branch December 3, 2021 11:46
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