-
-
Notifications
You must be signed in to change notification settings - Fork 28
chore: adding remaining showcases #123
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
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.
Awesome thank you!
So I read that there is no particular issue you are aware of, that prevents this component to be converted to alpinejs instead of |
…r showcase content
I've now fixed the issues and simply merged them! What additional rules do you think we can apply? What makes sense here? |
I am confused, you merged this branch , but I don't see this commit on main... I think it got overwritten ? |
This is totally weird. I don't know why this happened. I'll fix it immediately! |
So, I've reverted your changes with another PR. I'm so sorry about that. Apparently, I did a force push on some branch in my workflow that overwrote everything! I've now created additional main branch protection rules so this can't happen again! Thanks for your work! |
I added the rest of the showcases for the linter. List of failures.
Pretty much all of it is a duplicated
id
I fixed the one that was non-consequential, the rest of it is coming from the popover component. I see that it relies on an
id
for example here document.getElementById('popover-portal-container');My intuition is that anywhere it relies on id should be refactored to alpinejs, but I haven't looked closely into all of the logic for the component.
Alternatively we can exclude popover from the check, if you think it is a special case and has to be this way.
Either way lmk how you would like to proceed to get the check to green before we merge this.