merge-queue: embarking main (b45f5be) and #592 together #593
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
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
A few notes: * Our Tailwind config inverts the blue primary and secondary colors wrt. the document. (By my recollection, I think this was intentional and had something to do with the corresponding hover colors.) * Ann's document lists "red secondary" twice, once as #cd3764 and once as #f1685e. I believe #cd3764 was added late in the process and therefore I've called it "red tertiary" in this commit. * Ditto for "grey secondary," but in this case, the hex RGB value given for the 2nd occurrence of "grey secondary" does not match the color swatch. The hex value is the same as the 1st occurrence of "grey secondary" and therefore I've used the hex RGB value I measured by sampling the swatch in the document (which is #d4d4d4). I've called this "grey quaternary" in the Tailwind config.
This will become more useful in the next couple of commits.
We don't use all of these at the moment, but it's easy to forget these are necessary, so we might as well define them in advance.
("Mostly" because some of these nodes should be filled a solid color
with white text.)
The summary commit message describes the only behavioral change in
this commit, but I've buried the lede: this commit also majorly
refactors how we assign classes to nodes & edges in the tree by using
Tailwind CSS classes, rather than injecting raw `style` CSS
attributes.
At the moment, this doesn't buy us much and instead feels a bit like a
violation of DRY, but it does give us a lot more flexibility in
styling, which the next few commits will take advantage of.
The following flavors aren't selectable, and therefore we don't show hover for them: * `with` in `match ... with`. * Pattern boxes. * Pattern constructors. * Pattern applications. (Notably, pattern binds are selectable because we can rename them.) This implementation deviates from the design spec by making the width of the outline 4px, rather than 2px. I think 2px is a bit too thin, at least for our current overall style.
Per the Tailwind CSS docs, this is a 6px radius, which is what the UI design spec calls for.
I had been wondering separately what was the difference between `ring` and `outline` and finally stumbled across an explanation of why `ring` was added in the first place: tailwindlabs/tailwindcss#7649 It turns out that on Safari, outlines don't follow the border of the element they're outlining: they are always squared off. Tailwind CSS addressed this by adding `ring`, so for our purposes, this is what we should be using.
There's still some work to be done here, but this is a good approximation of the design spec. Among the things that remain to be done are: * We don't draw any labels on most syntax nodes. * The size of the label and the contrast need some work. * The single-glyph labels render like relatively flat ellipses rather than nice circles like the 3-glyph labels do. We might be able to fake this by padding out the single-glyph labels to 3 (spaces on either side), or perhaps with a bit of min-width magic.
With this commit, we pretty closely match the UI design spec for tree styling, with a few exceptions: * Holes are not rendered as a circle. * We don't have an `=` node for `let`s. * Value constructor and function `apply` nodes read just `apply` rather than `apply function`. We need to think about this a bit in the context of patterns, where `apply` only ever appears with a value constructor and we don't want students to get confused. Note that there are a few node types that weren't specified in the UI design spec (e.g., type constructors). At the moment, these are rendered in black, and we'll need to choose some colors for these. Also, nodes that appear in patterns are also rendered in black. I'm guessing this is a leftover from a debate we had a few months ago about whether patterns should be treated specially other than the box drawn around them. This will be corrected in a subsequent commit. Finally, the body content I've chosen here for some syntax nodes (e.g., `function type`) should be considered just placeholders for now. We should finalize the terms we use for these at a later date. (One complication is that they should be reasonably short, so that they don't take up too much space in the tree.)
Previously they were rendered in black.
|
The pull request #592 is mergeable |
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.
🎉 This combination of pull requests has been checked successfully 🎉
Branch main (b45f5be) and #592 are embarked together for merge.
This pull request has been created by Mergify to speculatively check the mergeability of #592.
You don't need to do anything. Mergify will close this pull request automatically when it is complete.
Required conditions of queue
defaultfor merge:base=maincheck-success="build-frontend"check-success="buildkite/primer-app/pr/required-nix-ci"check-success="deploy-to-chromatic"More informations about Mergify merge queue can be found in the documentation.
Mergify commands
You can also trigger Mergify actions by commenting on this pull request:
@Mergifyio refreshwill re-evaluate the queue rulesAdditionally, on Mergify dashboard you can:
Finally, you can contact us on https://mergify.com