-
Notifications
You must be signed in to change notification settings - Fork 147
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
Comments
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). 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)? |
@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? |
@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 |
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 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. |
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 |
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.
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. |
@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 |
@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. |
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
APII 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:
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. ImplementationAs 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:
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. |
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: 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 |
@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 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 |
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. |
@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. |
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. |
@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? |
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. |
From the docs:
In an ideal world, themes created for the node editor will:
This issue will serve as the umbrella discussion for theming-related issues and feature requests.
The text was updated successfully, but these errors were encountered: