cubing / cubing.js

🛠 A library for displaying and working with twisty puzzles. Also currently home to the code for Twizzle.
https://js.cubing.net/cubing/
GNU General Public License v3.0
232 stars 42 forks source link

[Feature request] Standalone controle panel elements, custom styling for components #316

Open bohdancho opened 5 months ago

bohdancho commented 5 months ago

Goal

I'd like to be able to

  1. use <twisty-scrubber> and <twisty-button>'s outside of <twisty-player>
  2. tweak web components' styles to my needs, including <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 a for attribute possible, like in TwistyAlgEditor and TwistyAlgViewer.

As for custom styling, here's a possible solution: image 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.

bohdancho commented 5 months ago

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 mode in the constructor: image

bohdancho commented 5 months ago

Another note: I also need cubing.js to export BoundaryType for me to be able to replicate control buttons (specifically playStepBackwards and playStep), would it be possible to export it as well? Or I would need a different way to to trigger these two actions, for example from twistyPlayer.controller

lgarron commented 5 months ago

The individual components can be used, but you should consider anything other than <twisty-player> itself to have a very unstable API. They're only publicly exposed because scoped custom elements (https://github.com/WICG/webcomponents/issues/716) aren't possible yet.

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: https://github.com/WICG/webcomponents/issues/909

That said, if you're willing to live with the caveat that the internals of any cubing/twisty elements (or even the existence other than <twisty-player>) could change at any moment, I'd be amenable for an opt-in CSS injection mechanism. This is how it looks using internal imports, but we could probably make it work:


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(`
/* … */
`),
);

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 a for attribute possible, like in TwistyAlgEditor and TwistyAlgViewer.

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.

Another note: I also need cubing.js to export BoundaryType for me to be able to replicate control buttons (specifically playStepBackwards and playStep), would it be possible to export it as well? Or I would need a different way to to trigger these two actions, for example from twistyPlayer.controller

Hmm, the API isn't really read to be exported, but you make a good point. I'd be alright with exporting BoundaryType as ExperimentalBoundaryType from cubing/twisty.

bohdancho commented 5 months ago

First of all, thanks for such a quick response!


I don't think it's a good idea to allow the elements to be open

Could you share your concerns about this? Allowing an options argument like { unstableOpenShadowDom: boolean } or shouldn't make any difference for anyone except those who are willing to opt into this unstable (hence the parameter name) behavior and deal with the possible consequences (like myself).


That said, if you're willing to live with the caveat that the internals of any cubing/twisty elements (or even the existence other than ) could change at any moment, I'd be amenable for an opt-in CSS injection mechanism. This is how it looks using internal imports, but we could probably make it work:

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 😄.

even the existence other than <twisty-player> could change at any moment

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?

If one of the goals requires a choice between doing work ourselves, or making a developer do it, we prefer doing the work so the developer doesn't have to


injectCSS(cssSource: CSSSource): void;

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?

const scrubber = new TwistyScrubber() as TwistyScrubber & ManagedCustomElementWithCSSInjection

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.

lgarron commented 5 months ago

Could you share your concerns about this? Allowing an options argument like { unstableOpenShadowDom: boolean } or shouldn't make any difference for anyone except those who are willing to opt into this unstable (hence the parameter name) behavior and deal with the possible consequences (like myself).

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.

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?

Well, the first bullet of that section ends with:

This also means it should be hard to "hold it wrong" and write unsafe/super slow apps by accident.

In the long term, yes, it's important that the building blocks of <twisty-player> can be composed. But we're still rather early in the design process.

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?

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.

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?

Not quite, I'm trying to demonstrate that ManagedCustomElement itself would not have a way to inject CSS until after setDebug() is called.

Would it be possible to opt into CSS injections in (actually) standalone components?

Hmm, I'm not quite sure what you mean by that.

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.

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.

bohdancho commented 5 months ago

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.

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?

In the long term, yes, it's important that the building blocks of can be composed. But we're still rather early in the design process.

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?

Not quite, I'm trying to demonstrate that ManagedCustomElement itself would not have a way to inject CSS until after setDebug() is called.

Does this mean that setDebug() would be used not only for debugging, but also to opt into CSS injections?

Would it be possible to opt into CSS injections in (actually) standalone components?

Hmm, I'm not quite sure what you mean by that.

Sorry I meant components that are meant to be used by developers (like <twisty-alg-viewer> or <twisty-alg-editor>) and are not just building blocks of another component.

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.

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.