-
Notifications
You must be signed in to change notification settings - Fork 1
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
Better styling #1316
Open
georgefst
wants to merge
38
commits into
main
Choose a base branch
from
georgefst/stylesheet
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Better styling #1316
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should be easier to work with than using inline styles for everything. It is also necessary for some functionality, e.g. changing styles on hover, which relies on a pseudo-selector and thus can't be used inline. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Note that we use an `isSelected` parameter for `viewTreeExpr` etc. instead of seemingly simpler approaches like passing a `Maybe ID` because we have to make everything work when the metadata type is `()`. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
…ypes and kinds Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
As well as simplifying the code, this gets rid of an unnecessary `div` layer. Signed-off-by: George Thomas <[email protected]>
We also remove click handlers from such nodes, which just triggered `NoOp "clicked non-interactive node"` anyway. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
We now use a partially-transparent ring for nodes which are only hovered but not selected. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
The code is a lot more flexible now that we pass around `NodeViewData` rather than always immediately passing it to `viewNode`. This will ultimately allow for things like better edge styling. Signed-off-by: George Thomas <[email protected]>
The major change here is making edges actual children of their source node in the DOM, allowing the colour to be inherited. The separate "node-contents" is necessary for this, but adds more flexibility which could be useful in future anyway, e.g. if we want labels on nodes like we had in the JS version. This commit also unintentionally solves in passing an issue where pattern edges were invisible due to bad z-indices. Signed-off-by: George Thomas <[email protected]>
We instead use CSS to center edges within the parent node. Signed-off-by: George Thomas <[email protected]>
This approach is more robust against future changes. Signed-off-by: George Thomas <[email protected]>
We now place trees within tight bounding boxes. This will make styling easier, including by removing an unnecessary `div` layer. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Oh, whoops, I've accidentally formatted the README. Just a sec... |
These are taken from `primer-app`, our old JavaScript frontend. We also copy licenses, and the section from the README about third-party code (including the part about vendoring, which isn't actually relevant to changes in this commit, but is appropriate due to the layout algorithm from diagrams). Note that we don't yet use the "branding" font, but we might as well copy all the fonts over together. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
We can almost get away with stripping the inner `div` and moving `overflow` etc. up a level, but we'd have to remove the flexbox, which makes centering the text vertically difficult (and would require further special-casing pattern boxes, though that's not a major issue). Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
It's a bit pointless right now, and removing it allows us to simplify things. When we layout the UI properly with components in grids we'll likely want to do things differently anyway. For now we apply some padding in to the whole canvas instead, via the stylesheet. Which at least means that hover rings don't overflow the screen. Signed-off-by: George Thomas <[email protected]>
They're no longer used as much as they once were, so the code is simpler without them. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
The offset is not obvious without setting `nodePadding` and `boxPadding` to 0. For some reason, the used/computed width for the element is otherwise less than the specified width. Changing to `min-height` as well is seemingly unnecessary but harmless, and it feels weird not to be consistent. Signed-off-by: George Thomas <[email protected]>
The plan was initially to go the other way, removing remaining `style_` tags, and working with computed values in the stylesheet somehow. Instead, we note that uses of CSS properties mostly fall cleanly in to two categories, and use the stylesheet solely for the latter: - Functionality: properties which are essential for the app to work. - Aesthetics: properties which could in theory be overridden by a user-specific "theme". We can remove these entirely and have an app which is still functional, if ugly. Another way to think about this is that if we were to separate out functionality in to React-style components, the former would be hardcoded to ensure reasonable encapsulation, and the latter configurable. This is a common pattern in libraries such as ReactFlow. In some cases, it might make sense for something to be overridden even when it is necessary that it has _some_ value. For example, without setting `border-style: solid` on edges they can't be seen at all, but a theme could still override with `border-style: dotted !important`. See also the flexbox properties for `node-contents`. In these cases, we use inline styles to preserve the principle that the stylesheet-less app still works. Signed-off-by: George Thomas <[email protected]>
One of our recent changes must have made this possible where it wasn't before, e.g. by removing intermediate `div`s. Signed-off-by: George Thomas <[email protected]>
It's not really designed for this (see comment on `clayToMiso`) but it actually works really nicely for avoiding manually constructing style strings and adding a degree of type safety. Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
Signed-off-by: George Thomas <[email protected]>
This makes no functional difference, but is good practice due to indicating the semantics as well as providing a theoretical performance improvement. Signed-off-by: George Thomas <[email protected]>
georgefst
force-pushed
the
georgefst/stylesheet
branch
from
January 9, 2025 20:13
119eae7
to
a5a6d45
Compare
Recent Wasm builds on edit: fixed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In a break from tradition, I would strongly recommend not attempting to review this one-commit-at-a-time. At this early stage of the frontend project, it's natural for there to be big sweeping changes, and it's the final state of this PR that's important, with the intermediate states best left as historical curiosities...
Given that we've said styling isn't the most important thing right now, I may have gotten a bit carried away here! But as well as taking us away from messy string concatenation for styles, this work has prompted several more general refactors, leaving the app in great shape for future work. Plus, some of the styling improvements are really about basic usability. For example, when I was working on a proof-of-concept for applying actions a few weeks ago, it was hard to debug without the current selection being indicated. Similarly, the recently-added definition panel buttons were previously way too small and annoying to click on, while simply making them bigger without adding scrollbars would have taken up too much screen space.
I believe I've developed a sensible set of principles for managing CSS, as expressed in 918eb41 (in summary: inline styles with Clay for component functionality, and
style.css
for pure aesthetics). This has so far been very pleasant to work with, but we may evolve our approach as we scale:style.css
is quite a long way from being interesting or repetitive enough for this to be of much benefit. This might start to change when, for example, we wish to implement the beginner and intermediate mode styles fromprimer-app
.Finally, note that we aren't yet attempting to perfectly match
primer-app
. For the trees, we're very close, and better in some ways, but for the definitions panel what we have here is basically the simplest thing that looks somewhat decent. Similarly, using scrollbars rather than a pan-and-zoom-able canvas isn't the best UX, but it's simple.