-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
Add option to set swup main element via data attribute #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line 40 (options.containers = this.element.getAttribute('data-containers').split(' ');
), the containers are overriden, which may create a weird behavior if you use both mainElement and containers.
WDYT we should do if both are present in the HTML?
That's a good question. If the attribute |
Yes, I think that would be the best course of action: 1/ if neither a main element nor containers are provided, use I think that makes the most sense Also, can you rebase on main as the controller changed a bit? |
I rebased my branch on the current main and implemented the required code. Do I have to build |
You should build it :) |
@tgalopin it might be worth improving the CI to ensure this does not get forgotten, if possible. |
Hey @optior ! I had a local look at your PR, it's great, thanks! The final thing that I think we can do it add tests for the various situations. In |
I'll give it a try on the weekend, never done any js testing before. |
Awesome, thanks! Feel free to ping me on Symfony Devs Slack if you have questions! |
Friendly ping @optior |
I have no clue how to write those tests. Unfortunately I have currently no time to dig into it. It would be great if anybody else could write these tests. |
Finally I got the tests working..but now I can't fix the linter errors. @tgalopin Are the tests ok this way? Any idea how I can fix the linter errors? |
Hey @optior! I'm sorry hasn't gotten more attention yet :/. Would you be willing to update it to the latest version? The big change is that we refactored away from Thanks! |
Hey @weaverryan, I'll update this pr in the next couple days. |
Hey @weaverryan, I just pushed the updated files. |
Thank you @optior! And nice additions to the tests too :) |
This pull request adds the possibility to change the
mainElement
of the swup theme. For this purpose the new data attributedata-main-element
is introduced.