Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move getElementFromSelector & getSelectorFromElement #36027

Merged
merged 2 commits into from Nov 6, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 16, 2022

Move getElementFromSelector & getSelectorFromElement inside selector-engine.js, in order to use SelectorEngine methods, avoiding raw querySelector usage

Feature: add getMultipleElementsFromSelector helper

@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from d84c041 to 7d5daed Compare March 19, 2022 00:00
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from 7d5daed to a4b16f3 Compare March 28, 2022 22:02
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch 4 times, most recently from edde81f to c7721e5 Compare April 14, 2022 11:23
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from c7721e5 to d98e376 Compare May 10, 2022 21:43
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from d98e376 to f3e62e9 Compare May 19, 2022 23:10
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from f3e62e9 to 0b54300 Compare June 13, 2022 07:32
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from 0b54300 to 79ed268 Compare July 16, 2022 22:19
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch from 79ed268 to 3f36e65 Compare July 20, 2022 09:14
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch 2 times, most recently from ca34019 to 4f38b46 Compare August 31, 2022 14:17
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

PR LGTM but I got a question of architecture.

For the Manipulator we can do stuff like Manipulator.getDataAttributes(). We don't want here to be able to do SelectorEngine.getElementFromSelector(), SelectorEngine.getMultipleElementsFromSelector() and SelectorEngine.getSelectorFromElement()?
It would probably allow to export it like export default SelectorEngine and so not being forced to import it with brackets like we had to do for example in import { SelectorEngine } from '../../../src/dom/selector-engine'. And would be consistent with all the files in js/src/dom/*.js that are built like this.

Was thinking of something like that: patch.txt (git apply patch.txt to see the diff)

@GeoSot
Copy link
Member Author

GeoSot commented Oct 20, 2022

Of course, I am ok with this approach. I tried to do the minimum changes.
I have applied your suggestion on e96546d

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

This change LGTM! I let others double-check that :)

@GeoSot GeoSot marked this pull request as ready for review October 20, 2022 15:56
@GeoSot GeoSot requested a review from a team as a code owner October 20, 2022 15:56
@GeoSot GeoSot force-pushed the gs/moving-selector-helpers branch 2 times, most recently from cbddc6f to c185ccf Compare November 1, 2022 14:02
Move `getElementFromSelector` & getSelectorFromElement` inside selector-engine.js, in order to use SelectorEngine methods, avoiding raw querySelector usage

Feature: add `getMultipleElementsFromSelector` helper
@GeoSot GeoSot merged commit e81e7cd into main Nov 6, 2022
@GeoSot GeoSot deleted the gs/moving-selector-helpers branch November 6, 2022 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants