-
Notifications
You must be signed in to change notification settings - Fork 48
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
[Feature request] Standalone controle panel elements, custom styling for components #316
Comments
I found a simpler solution. It feels a bit hacky, but doesn't require an API change. A downside is that it requires manual overriding of default styles provided by cubing.js. const scrubber = new TwistyScrubber(twistyPlayer.experimentalModel, twistyPlayer.controller, {
const style = document.createElement('style')
style.innerHTML = scrubberStyles
scrubber.addElement(style) While this does work, it turns out I also need JS level access to Shadow DOM (to style lower and upper fill in the range input track like here https://toughengineer.github.io/demo/slider-styler/slider-styler.html). For this we could allow passing |
Another note: I also need cubing.js to export |
The individual components can be used, but you should consider anything other than I don't think it's a good idea to allow the elements to be open, and unfortunately we are far from having an ecosystem solution to allow styling custom elements from the outside: WICG/webcomponents#909 That said, if you're willing to live with the caveat that the internals of any import { setTwistyDebug } from "cubing/twisty";
import {
CSSSource,
ManagedCustomElement,
} from "cubing/twisty/views/ManagedCustomElement";
import { TwistyScrubber } from "cubing/twisty/views/control-panel/TwistyScrubber";
setTwistyDebug({
allowManagedCustomElementCSSInjection: true,
});
interface ManagedCustomElementWithCSSInjection extends ManagedCustomElement {
injectCSS(cssSource: CSSSource): void;
}
const scrubber = new TwistyScrubber() as TwistyScrubber &
ManagedCustomElementWithCSSInjection;
scrubber.injectCSS(
new CSSSource(`
/* … */
`),
);
I don't think we're quite ready for that, but I'd be willing to take a look at a PR for this, if you think you can come up with a good way to implement this.
Hmm, the API isn't really read to be exported, but you make a good point. I'd be alright with exporting |
First of all, thanks for such a quick response!
Could you share your concerns about this? Allowing an options argument like
I'm more than willing to live with this caveat because the previous iteration of the feature I'm working on used a canvas that imports my frankenstein fork of https://alg.cubing.net 😄.
Making the scrubber component private (or making it unusable from the outside in some other way) without an alternative would mean that the only option left is reimplementing it. Wouldn't this go against the following goal from Goals and Principles?
For this to work CSSSource must also be exported. Is there a problem with accepting a string and creating a CSSSource internally inside of injectCSS?
I'm not sure I understood your suggestion correctly, shouldn't the TwistyScrubber class itself extend from ManagedCustomElementWithCSSInjection? If so, why would we need type casting? Would it be possible to opt into CSS injections in (actually) standalone components? The one I need to customize is TwistyAlgViewer, which is a little trickier to implement CSS injections for, because its leaves have their own shadowRoots. |
This is quite risky — the web is full of APIs where someone exposed the internal implementation just to get things going and we're still paying the price. As a security professional, I'm also constantly wary of APIs that say "only use this if you know what you're doing" because not everyone is good a judging how much they know what they're doing. It's one thing to expose specific functions and accessors that will shift around a bit, but it's another to expose the entire DOM tree and I don't want this to even possibly cause issues.
Well, the first bullet of that section ends with:
In the long term, yes, it's important that the building blocks of
Well, what this should really be using is constructable style sheets. I would have used those, but they weren't ready when I first wrote this code. A string should be fine for any page with a realistic number of players and moderate amounts of styling, though.
Not quite, I'm trying to demonstrate that
Hmm, I'm not quite sure what you mean by that.
Do you know what styling you need to do? This is one that I do want to provide some CSS styling variables for, but I don't know if it would be enough to support your use case. |
I agree with your point about dangers of exposing the internal implementation. Would you be amendable for exposing just the input DOM element? inputElem.style.setProperty would be enough for my use case too, but exposing just this particular handle seems more like a band aid than a sustainable solution, because tomorrow someone might need a different method, then another one, and so on. Maybe we could take inputElem.style as the middle ground?
Would it be possible that you don't convert it to a custom scoped element until there's an alternative way for me to use it? Or would you rather have me build the component myself, should this be the case?
Does this mean that setDebug() would be used not only for debugging, but also to opt into CSS injections?
Sorry I meant components that are meant to be used by developers (like
Background color of the current move, background color on hover/active (instead of changing the text color), removing text underlining on hover/active for moves. |
Goal
I'd like to be able to
<twisty-scrubber>
and<twisty-button>
's outside of<twisty-player>
<twisty-scrubbler>
Each of the buttons just calls a controller method, so replicating them even without standalone components is pretty straightforward.
<twisty-scrubbler>
on the other hand is more sophisticated so manually replicating it's functionality is more error-prone (as the internal API and the original component might change in the future).Possible solution
Including
scrubble
in exports shouldn't be an issue itself. While I'm fine with passing model and controller to the constructor in JS, we would also need to add attributeChangedCallback to make using it via afor
attribute possible, like inTwistyAlgEditor
andTwistyAlgViewer
.As for custom styling, here's a possible solution:
It is worth noting that passing custom css while leaving default styles enabled results in two
<style>
tags, tbh idk if it's a problem.Alternatives
Alternatively we could abstract away the custom styling logic and the binding logic in a class like
TwistyStandaloneElement
which would extend from ManagedCustomElement. I would be happy to provide a pull request with these changes.The text was updated successfully, but these errors were encountered: