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

🚀 Theming Umbrella Discussion #18

Open
chrisjpatty opened this issue Aug 6, 2020 · 17 comments
Open

🚀 Theming Umbrella Discussion #18

chrisjpatty opened this issue Aug 6, 2020 · 17 comments

Comments

@chrisjpatty
Copy link
Owner

chrisjpatty commented Aug 6, 2020

From the docs:

Any quality library which provides a user interface for end users must be theme-able to match the rest of the application. Flume is no exception to this, and work is underway to provide a stable API for creating node editor themes. Because of the diverse methods that developers use to style their applications, we are working to make sure when this API is released it will be robust and universal.

In an ideal world, themes created for the node editor will:

  • Have full access to overriding CSS for nodes, ports, controls, and the stage itself.
  • Be able to adjust the styling of connections, potentially including the way in which they are rendered.
  • Be easily shareable with the community.

This issue will serve as the umbrella discussion for theming-related issues and feature requests.

@andraz-at
Copy link

When rendering Flume NodeEditor as a subwindow of a bigger app, general appearance needs to blend in with the rest to look professional.

I'm trying to override the background styling, it is simple to do in the inspector, just turn off few css checkboxes. How to do this with props?

Override makes it blend seamlessly with the current styling (the borders are draggable, coming from react-mosaic, and NodeEditor resizing to fit 100% of the window works like a charm out of the box).

image

p.s.: How to trigger ":hover" on drop zone when dragging a connector over it (maybe alter hover color, calculate hsl change l +10 by default)?

@chrisjpatty
Copy link
Owner Author

@andraz-at There isn't a standard way to override the styling yet, but that's coming soon. I've been debating what the right API should be, and there's a few considerations. I'm thinking you could provide a theme config object with overrides for different elements like the background, but that limits what CSS changes you can make. Or I could attach class names that won't change between renders/builds so you can fully override the theme, but it's more manual.

Since you're trying to override the styles, what would be your preferred way of doing so?

@chrisjpatty chrisjpatty pinned this issue Aug 13, 2020
@chrisjpatty
Copy link
Owner Author

@andraz-at Also, on the hover idea, I think that's a feature that definitely needs to be implemented. One of the challenges is adding that hover state but only while the user is dragging a connection. For performance reasons the editor doesn't re-render while the user is dragging, so I need to add a mechanism to notify ports that the user is dragging, without requiring a re-render. It would also need to conditionally apply the effect if the port being hovered will accept the connection. All that's to say that it's unfortunately not as simple as adding a :hover pseudo class to the ports, but it's a feature that would definitely enhance the user experience and should be implemented soon.

@andraz-at
Copy link

I see theming/styling dilemma as the "css !important clobbering vs javascript style override", but we could simply have both available.

Sometimes it is way simpler to just clobber a class and be done with it (generated classnames need to be static and unique + general enough to target all nodes/connectors/etc., so multiple space separated classes would be the best bet: one calculated from the props/config of that specific item, one generic for general group styling) this would be a welcome feature for fast prototypes on codesandbox and similar.

As all blunt tools, this can do more damage than good when embedded in a complex app with nested child nodes (like Flume has when you create custom nodes - what if they contain a complex widget with own styling which now gets clobbered?).

Here is where you need precision and use the configuration override prop. It can trivially implement the whole CSS stack, like in this exercise: https://www.w3schools.com/js/exercise_js.asp?filename=exercise_js_dom_html6

You simply apply any .style. property which got defined in the theme prop: {color:'red'} ➡ domobj.style.color = 'red' when the dom object gets created (but before being added to the dom, to prevent repaints).


Regarding hover: if action performs below 16ms you're not breaking 60 fps.

Action below should perform <1ms for over 1000 nodes, so toggling 50 is trivial as CSS engine is super optimized when it comes to class changes.

When you wish the effect to work, append to all nodes a class that has the :hover set in the css. When you wish to disable the effect, remove that class.

@andraz-at
Copy link

andraz-at commented Aug 14, 2020

My preferred way of styling would be with the override prop, which would apply the extra styling when element gets created.

At the moment I'm using this (which looks quite fragile):

.Stage_wrapper__1X5K_ {
  background: transparent;
}

.Stage_wrapper__1X5K_ textarea {
  background: white;
  border-color: #ccc;
}

.Node_wrapper__3SmT7 {
  background: white;
}

.Node_label__3MmhF {
  background: white;
}

.Select_selectedWrapper__SUs4D {
  background: white;
  border-color: #ccc;
}

to get

image

@PhilGarb
Copy link
Contributor

PhilGarb commented Aug 16, 2020

I would go with a CSS in JS solution like emotion or styled components, because they are very flexible and allow multiple options for an API. When an object is used it can be merged with default styles (this would also mean that default styles that are not targeted won't be modified). When a css class is used the order of applied classes can be controlled by following a composition pattern instead of using something like important.

I'm thinking you could provide a theme config object with overrides for different elements like the background, but that limits what CSS changes you can make. Or I could attach class names that won't change between renders/builds so you can fully override the theme, but it's more manual.

By using CSS in JS you would have the full power of css when using objects as well as the option to go with class names without important.

I think your first idea hints at somewhat of a theming solution. I would not go for any solution that does not at least have the option to configure everything. Constraining the properties that can be customized should be a "feature" in that it allows you to skip the manual work if you don't need full customization. In CSS in JS land there are a couple of solutions for theming and also a specification you could follow. By following that you could have a default theme that can be customized via props, but also respects an already present theme.

@chrisjpatty
Copy link
Owner Author

chrisjpatty commented Aug 17, 2020

@PhilGarb I agree, the more powerful solution is probably the better one. Under the hood it's using Post CSS Modules for styling already, which just imports an object of generated classnames that are hooked up to the elements. However it doesn't work similar to something like Emotion which lets you add dynamic CSS at runtime. What do you think about supplying a object of class name mappings, something like this:

{
  node: "node-class-name",
  nodeTitle: "node-title-class-name",
  port: "port-class-name",
  connection: "connection-class-name",
}

This mapping would then be applied to the various elements as the second class name so you could override whatever you want. There's some downsides to this approach though, specifically you would still need to follow the rules of specificity for any selectors in the node editor which may be overriding your top-level class name. It also means if you're trying to override everything that there potentially needs to be a LOT of available keys available on this mapping object.

If the current styling solution were replaced with something that accepts dynamic runtime styles like Emotion, the CSS prop could be used to add arbitrary styles to different elements, but it would likely be a big refactor, and support for theming capabilities may rely on the developer adding a babel plugin to their build pipeline which isn't ideal, and may not be possible for all projects.

One other possibility could be adding data attributes to all of the stylable elements. Using data selectors in CSS would already be more specific than classnames I believe. So nodes could have data-flume-node, and it could be selected in the host CSS as [data-flume-node]{ //styles }. But like you've brought up, in some cases it may necessitate using !important on occasion, which isn't ideal.

@PhilGarb
Copy link
Contributor

@chrisjpatty definitely a possible approach especially if it is easy to implement. I do however see the issue of it becoming a very large object, that might be hard to maintain alongside the library in general (constantly having issues created, because some class here and there is not applied would be annoying). When sticking to CSS classes is important I have seen other component libraries use one css stylesheet that is imported alongside the library. This sheet could serve as a reference for what classes are available and if a user wants to customize styles he/she can just edit the one stylesheet. This avoids the need for any objects or styling abstraction in JavaScript. And would propably be the easiest to implement, because it only means merging the CSS into one readable stylesheet which is most likely more of a build tool configuration then an actual refactor of the code.

While thinking about a good theming solution I am always wondering about the general way custom nodes can be added to flume. From my current understanding the only way to create them is through the config object. Is that correct? If it is why not extend the API of addPortType and addNodeType to accept a CSS class? It could then be up to the user how the CSS is written or generated.

Why would theming require a babel plugin? The NodeEditor could just accept a theme based on a certain scheme and internally something like emotion is used to wrap the NodeEditor with that theme.

Data attributes solve the specificity issue, but not the cascade so, as you said, important might become necessary in some places.

Maybe a better approach would be to first define the requirements of the theming solution instead of trying to find the perfect solution (that probably does not exist) from the get go. Then we could create a list of requirements and see which solution solves most of them and rank the solution based on that and the time it would take to implement. If CSS in JS would be the best solution I could support you in refactoring. I had a look at the code base and since your styling is already very modular it shouldn't be hard to refactor just a little tedious.

This was referenced Aug 21, 2020
@PhilGarb
Copy link
Contributor

I have thought a little about this issue in the past couple of days. I think there are three main use cases the library should support in regards to styling.

Use Cases

  1. Adjusting styles to fit the editor into the environment. Eg. colors, fonts or spacing
  2. Logic based styles. I see the use case of combining business logic with styling like already raised here Color connecting lines relative to boolean state #30 . This could allow for features such as coloring ports or connections based on their validity or coloring nodes when content is missing.
  3. Lastly there might be more general adjustments a library consumer would like to make, like changing the tree direction from left to right to top to bottom or change how text is arranged on a node.

API

I think number 3 is important, but should not be part of the theming discussion, because a consumer might not even know how to achieve these overarching adjustments. I would group this with props like disableZoom, disablePan ... as a prop of the NodeEditor.

Number 1 and 2 are the features I would like to see supported as part of the theming of the editor. The API could work like this:

  1. A theme is passed to the NodeEditor. We could either define the theme as an object or a css class with css variables defined. I think it is important to have one unified way to send the NodeEditor all the styling tokens instead of keeping many different classes consistent.
  2. Internally we could then base the css on the variables instead of hardcoding values for colors, fonts or spacing. We would also need to document precisely which variable is used where. This is however not so different from applying classes where the documentation would need to show which className goes where. This would already allow for a great deal of customization just by changing certain tokens and might already be enough for most use cases.
  3. Logic based styling would best be associated with individual ports and nodes. I would therefore extend FlumeConfig's addPortTypes and addNodeType with a styles property. This should accept an object with styling properties and functions to apply an individual style. Tokens from the general theme can then be referenced to keep the look of the library consistent. The functions could then be used to determine the styling value based on the internal state of the editor, port or node. (I have deliberately not covered the second missing feature raised by chrisjpatty in Color connecting lines relative to boolean state #30:
  1. Live logic resolution in the Node Editor. Right now the node editor has not built-in concept of what data is coming from input nodes, so a boolean port may not know in the editor if the data flowing through it is a true or a false. Unless of course you're intending to have a true and a false output port.

I consider this to be a separate requirement to implement logic based styling. The theming solution however should just be able to accept a function to make something like #30 possible.

Implementation

As I have stated before I believe, that a CSS in JS solution would be the most optimal solution as it would allow us to handle different pain points like:

  • controlling CSS specificity

  • allowing for an easy way to share a theme

  • controlling styling both internally and through the API based on state

  • it would also make something like PostCSS unnecessary, because this would already be included

While this would be somewhat of a refactoring I do believe this would be worth it to gain full flexibility for the theming solution. Afterwards a component could look like this (based on the Draggable component in Node.js):

//Note this assumes the theming prop being passed an object an not a css class.

const CustomNode = props => {
  return (
    <Draggable
      css={theme =>({
        background: theme.color.primary,
        borderRadius: theme.radius[2],
        boxShadow: "0px 4px 8px rgba(0, 0, 0, 0.4)",
        position: "absolute",
        left: "0px",
        top: "0px",
        userSelect: "none",
        display: "flex",
        flexDirection: "column",
        zIndex: "1",
        cursor: "default"
      })}
      {...props}
    >
      {props.children}
    </Draggable>
  );
};

Please let me know what you think and whether you have more use cases or feedback about the API and implementation.

@mcamac
Copy link

mcamac commented Sep 15, 2020

Hi all - great library! I just wanted to second the CSS-in-JS comments above and share a couple CSS-in-JS approaches to subcomponent styling in the wild:

  1. react-select
  2. Chakra UI (beta)

These APIs (which are basically a translation of the above CSS subcomponent/BEM styling into CSS-into-JS) might work well for theming the default widgets and controls.

Edit: I think both libraries also support classnames as well: react-select

@chrisjpatty
Copy link
Owner Author

chrisjpatty commented Sep 28, 2020

@PhilGarb My apologies for the late follow-up, I've had some tight deadlines at work. Thank you so much for putting together this writeup. It helps formalize a lot of my thoughts around this topic. I agree with you that there needs to be a first-class standard method for theming, and a CSS-in-JS approach is probably the right way to go. On that front my preferred library would probably be emotion using their styled component wrapper. It would definitely be ideal to have themes created by users be easily sharable, so we can slowly build a repository of user-contributed themes, and focusing theming around providing a theme object to the component that emotion or another CSS-in-JS library consumes, is probably a good path forward for supporting that.

Off the top of my head I can think of a few styling variables that could be easily lifted to a theme object. The easy ones are the colors, and then shadow styles and fonts. There also may be a couple of places where the font stack has been repeated, and it should probably be refactored to ensure that all components just inherit a font stack that's set at the top level.

The logic-based styling is the trickier use-case and it's probably a lower priority than being able to statically restyle the components. A path forward might be to focus first on converting the CSS modules to emotion (unless there's a compelling argument for a better CSS-in-JS library), and lift all the variables to a top-level theme object.

@PhilGarb
Copy link
Contributor

PhilGarb commented Oct 5, 2020

Hi @chrisjpatty to my knowledge emotion is striking a good balance between a nice API and performance/bundle size. There are more static alternatives that disappear during bundling and provide therefore no overhead, but I think we are introducing the CSS in JS solution specifically for the runtime so these do not really make sense to me.

Would you like a PR of the modules refactored into emotion? I think the initial work is more or less copy and paste. The question is which style you would prefer, since emotion provides both a styled tagged template function and the object approach through a css prop. These do not have any Implication on the user facing API, but I would like to use what you prefer, since it is your codebase.

Lastly if we go ahead with introducing this theming/styling approach I would recommend opening a new issue to discuss which properties should be part of a theming object in addition to the ones you have already mentioned.

@chrisjpatty
Copy link
Owner Author

@PhilGarb If you want open a PR for moving styles to emotion that would be great, thanks for the offer! And to be clear, I'm totally open to exploring other CSS-in-JS libraries if you think something might be better than Emotion. I've had good experiences with Emotion in the past especially with their tagged-template literal api so you can use real CSS syntax. I agree with your first statement, I think by nature of this component, and the potential for styles depending on live data, makes a static css file less practical than runtime-generated CSS, but I'm open to other options we haven't considered.

@astrealRBLX
Copy link

astrealRBLX commented May 7, 2021

It's been a year since this issue was created with still nothing done. Still using the hacky solution of setting the css of the various obfuscated class names to achieve a somewhat nicer design. Any news on this?

Adding to this to say that I don't why when rendering components having a default variable for styles that can be overridden by the user is a problem.

@chrisjpatty
Copy link
Owner Author

@astrealRBLX Apologies that I haven't been able to put much attention towards this. My most recent plan is to add data-attributes for each element, that would provide a stable selector for you to use. My thinking is that that's the quickest path to making the styles overridable in a stable way, and then in the future we could explore more complex solutions like CSS in JS. Would that pattern work for you?

@Perpaterb
Copy link

Perpaterb commented May 12, 2022

Hi

I love flume BTW. Thanks a lot.

I would love it if I could theme based on the node type and connection type.

Just adding the type as another class would work for me I think.

Thanks.

@Perpaterb
Copy link

:) Not ideal

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants