feat(components): a simple HTML-based Tree component #112

brprice · 2021-10-28T12:13:21Z

We add a tree render which emits plain html trees, as nested lists.

brprice · 2021-10-28T12:15:07Z

NB: this PR implements a simplified version of our current tree renderer from vonnegut, mostly as a learning exercise / to check that CI works as I expect. I don't know if we want to actually commit it, and if we do it probably needs some polish.

The example trees are exported from primer 11e2c47a80422b391691e5da5bbd1ef809ca5703 (some version of hackworthltd/primer#180), but have been manually edited in accordance with comments on that pr:

label lambdas with variable names "Lam x" rather than "Lam"
call children childTrees rather than children

dhess · 2021-10-28T12:59:39Z

I actually think these could be perfect for a screen reader implementation. I've previously mentioned wanting to keep around the Vonnegut tree rendering style for that reason, but this one would be much easier to maintain and just as good from an a11y perspective, I would imagine.

brprice · 2021-11-15T16:51:56Z

I actually think these could be perfect for a screen reader implementation. I've previously mentioned wanting to keep around the Vonnegut tree rendering style for that reason, but this one would be much easier to maintain and just as good from an a11y perspective, I would imagine.

You mentioned that for a11y purposes, probably the first, "outline"/"nested <ul>" implementation would be just as good. Should we then drop the second commit, reword the first (to avoid saying "starting") and then merge?

dhess · 2021-11-15T16:53:47Z

I actually think these could be perfect for a screen reader implementation. I've previously mentioned wanting to keep around the Vonnegut tree rendering style for that reason, but this one would be much easier to maintain and just as good from an a11y perspective, I would imagine.

You mentioned that for a11y purposes, probably the first, "outline"/"nested

" implementation would be just as good. Should we then drop the second commit, reword the first (to avoid saying "starting") and then merge?

Yeah, I think that's my preference for now. Perhaps you should keep the current branch head around somewhere in case we want to return to the "pretty" HTML version of the tree, but personally, I think the outline-ish version in your original commit was more interesting for the long term.

If nothing else, it means it will be easier to maintain with less styling.

brprice · 2021-11-16T12:45:23Z

I have moved the "fancy css tree" to https://github.com/hackworthltd/primer-app/tree/brprice/tree-html-fancy , and this PR is now just the "outline" tree.

dhess · 2021-11-16T12:48:08Z

Good idea. I'll review this shortly.

brprice · 2021-11-16T12:53:05Z

See my comment on the storybook https://www.chromatic.com/test?appId=61562fccd90e96003afe472a&id=6193a7f67b9bc0003a9d719c -- it seems things are getting cut off, but I'm not sure why this is.

dhess · 2021-11-16T13:11:27Z

Ugh, I can't build this on my aarch64-darwin because this branch predates the fixes for that arch. Can you rebase this on main so that I can review it locally? Sorry for the hassle, but I suppose it needed to be rebased anyway.

dhess

Let's move the Tree core type into @hackworthltd/primer-types, but otherwise, this looks good. Thanks!

packages/primer-components/src/Tree/Tree.tsx

dhess · 2021-11-16T13:25:44Z

packages/primer-components/src/Tree/Tree.tsx

+  </ul>
+);
+
+function ChildTrees({ trees }: { trees: TreeI[] }): JSX.Element {


It's not specific to this PR, but it seems like a good time to bring it up, as it's still early days and now's a good time to have a policy.

Personally, I really dislike this distinction between function and const. Should we use function consistently? It seems to be anti-vogue, but I confess I don't understand why, exactly.

TBH, I don't really understand the difference (there seems to be some difference in "hoisting"/scoping: it seems that functions are visible through the entire file, but consts are only visible below where they are defined? Also you can apparently re-bind functions in JS, so const is perhaps slightly safer?). Mostly I use whichever one makes the linter not complain, defaulting to const for simple things as const Foo = () => <Component ... /> is easier than function Foo () { return <Component ... /> }.

The linter has a million options, so we could probably disable that specific complaint.

Anyway, I guess we should just go with the flow for now.

dhess · 2021-11-16T13:27:34Z

packages/primer-components/src/Tree/Tree.tsx

+      </ul>
+    );
+  }
+  return <></>;


(Parenthetical: return <></>; seems like something we'll need fairly often. I wonder if there's a way to define a JSX constant value for <></>.)

dhess · 2021-11-16T13:29:13Z

packages/primer-components/src/index.ts

@@ -9,4 +9,5 @@ export { default as SearchBar } from "@/SearchBar";
 export { default as SessionList } from "@/SessionList";
 export { default as SessionPreview } from "@/SessionPreview";
 export { default as SessionsNavBar } from "@/SessionsNavBar";
+export { default as Tree } from "@/Tree";


FYI, I've been doing this for each new type, too, but I suspect we'll need to export each component's properties as well in the index.ts file, in order to make it useful. Anyway, that's out of scope.

dhess · 2021-11-16T13:47:41Z

Oh, minor meta-nit: I believe that Conventional Commits requires a : between the "type" and the description: https://www.conventionalcommits.org/en/v1.0.0/#summary. However, you've elided that here, so it's probably worth fixing the title of this PR.

FYI, I think the way Bors works is that it uses the title of the PR and the contents of the first comment in its commit message, so there's not necessarily any need to force-push this change/typo fix to any of your git commits.

Also, to clarify: we're not enforcing Conventional Commits yet, but I think I will want to do that soon-ish, as I think it works pretty well and really the whole point of it IMO is to get some nice automation & tooling out of it.

dhess · 2021-11-16T13:50:51Z

One last point about this PR. I think this will be a great basis for a screen reader implementation of Primer's canvas. However, we'll probably want to at least decorate the tree with nicer names than Lam x etc. We can leave that for later, but I do want to come back to it at some point.

(edit In fact, one potentially nice, modular approach might be to pass the Tree component here a rendering function that it calls on each node. Then we'd have one renderer for screen readers, one for a mathematical/abstract kind of tree rendering, one that emulates a hand-drawn tree, etc. This is more or less what I had in mind when I talked about an abstract render target for trees in Vonnegut. However, I suspect I'm being too naïve about how such a renderer might work. It seems plausible that the renderer might want to process whole subtrees at once, rather than just node-at-a-time. On the other hand, that might be a justification for a multi-pass/compositional rendering system.)

packages/primer-types/src/Tree.ts

brprice · 2021-11-17T13:48:28Z

One last point about this PR. I think this will be a great basis for a screen reader implementation of Primer's canvas. However, we'll probably want to at least decorate the tree with nicer names than Lam x etc. We can leave that for later, but I do want to come back to it at some point.

Yes. I consider this out-of-scope here though.

brprice · 2021-11-24T12:39:11Z

bors try

ghost · 2021-11-24T13:54:14Z

try

Timed out.

dhess · 2021-11-24T14:07:39Z

Hmm, I'm not sure why that timed out. It appears it succeeded on the Hydra: https://hydra.hackworth-corp.com/eval/380650

brprice · 2021-11-24T16:15:51Z

bors merge

112: feat(components): a simple HTML-based Tree component r=brprice a=brprice We add a tree render which emits plain html trees, as nested lists. Co-authored-by: Ben Price <[email protected]>

ghost · 2021-11-24T16:17:17Z

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

{"message":"2 of 3 required status checks are expected.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

brprice · 2021-11-24T16:37:25Z

I don't know why bors failed (maybe it timed out before hydra finished?). Let's try again
bors merge

112: feat(components): a simple HTML-based Tree component r=brprice a=brprice We add a tree render which emits plain html trees, as nested lists. Co-authored-by: Ben Price <[email protected]>

ghost · 2021-11-24T16:39:00Z

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

{"message":"2 of 3 required status checks are expected.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

We render it in a simple nested list format. This is intended to be simple to maintain and a good basis for an accessibility-focussed component.

This unfortunately involves a lot of renaming churn, since the openapi description calls it 'Tree', rather than 'TreeI'. However, this is a good thing to do anyway, since having the bare-bones outline component being called 'Tree' would be confusing when we have some other, fancier, tree component as well.

brprice · 2021-11-24T16:43:09Z

I have rebased onto main. Let's see how bors is feeling
bors try

dhess · 2021-11-24T16:45:46Z

Ahh, I think perhaps at least some of our mysterious Bors merge failures (on this repo, anyway) are due to the fact that Bors doesn't know about the Chromatic jobs. So if Bors creates a new rev in order to merge, even if Bors succeeds, GitHub rejects the merge because the Chromatic jobs haven't run.

I don't think there's any way to teach Bors that if it does its own merge that it also needs to start Chromatic jobs, so probably bors merge commands on branches that aren't up to date won't work on this repo.

brprice · 2021-11-24T17:46:35Z

bors merge

ghost · 2021-11-24T17:47:45Z

Build succeeded:

ci/hydra:hackworthltd:primer-app-bors-staging:required

ghost · 2021-11-24T17:49:11Z

try

Timed out.

brprice force-pushed the brprice/tree-html branch from 8f0f7ad to 39f0314 Compare October 28, 2021 12:55

dhess assigned brprice Nov 8, 2021

brprice force-pushed the brprice/tree-html branch from 39f0314 to e528896 Compare November 16, 2021 12:44

brprice changed the title ~~feat(components) a HTML-based Tree component~~ feat(components) a simple HTML-based Tree component Nov 16, 2021

brprice requested a review from dhess November 16, 2021 12:45

brprice force-pushed the brprice/tree-html branch from e528896 to 5c042eb Compare November 16, 2021 13:14

dhess approved these changes Nov 16, 2021

View reviewed changes

brprice changed the title ~~feat(components) a simple HTML-based Tree component~~ feat(components): a simple HTML-based Tree component Nov 17, 2021

brprice force-pushed the brprice/tree-html branch from 5c042eb to 6cc26e1 Compare November 17, 2021 13:44

dhess reviewed Nov 17, 2021

View reviewed changes

packages/primer-types/src/Tree.ts Show resolved Hide resolved

ghost pushed a commit that referenced this pull request Nov 24, 2021

Try #112:

7a6f3b9

ghost pushed a commit that referenced this pull request Nov 24, 2021

Merge #112

9bb53a4

112: feat(components): a simple HTML-based Tree component r=brprice a=brprice We add a tree render which emits plain html trees, as nested lists. Co-authored-by: Ben Price <[email protected]>

ghost pushed a commit that referenced this pull request Nov 24, 2021

Merge #112

a16b1f9

112: feat(components): a simple HTML-based Tree component r=brprice a=brprice We add a tree render which emits plain html trees, as nested lists. Co-authored-by: Ben Price <[email protected]>

brprice added 3 commits November 24, 2021 16:41

feat(components): a Tree "outline" component

bece515

We render it in a simple nested list format. This is intended to be simple to maintain and a good basis for an accessibility-focussed component.

refactor: move TreeI type to primer-types

96cbfe9

brprice force-pushed the brprice/tree-html branch from 6cc26e1 to 708100f Compare November 24, 2021 16:42

ghost pushed a commit that referenced this pull request Nov 24, 2021

Try #112:

51fd7fc

ghost merged commit cdb7089 into main Nov 24, 2021

ghost deleted the brprice/tree-html branch November 24, 2021 17:47

brprice mentioned this pull request Dec 7, 2021

Do we need to port the existing tree renderer to React? #11

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(components): a simple HTML-based Tree component #112

feat(components): a simple HTML-based Tree component #112

brprice commented Oct 28, 2021 •

edited

Loading

brprice commented Oct 28, 2021 •

edited

Loading

dhess commented Oct 28, 2021

brprice commented Nov 15, 2021 •

edited

Loading

dhess commented Nov 15, 2021 •

edited

Loading

brprice commented Nov 16, 2021

dhess commented Nov 16, 2021

brprice commented Nov 16, 2021

dhess commented Nov 16, 2021 •

edited

Loading

dhess left a comment

dhess Nov 16, 2021

brprice Nov 17, 2021

dhess Nov 17, 2021

dhess Nov 16, 2021

dhess Nov 16, 2021

dhess commented Nov 16, 2021 •

edited

Loading

dhess commented Nov 16, 2021 •

edited

Loading

brprice commented Nov 17, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

dhess commented Nov 24, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

brprice commented Nov 24, 2021

dhess commented Nov 24, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

ghost commented Nov 24, 2021

feat(components): a simple HTML-based Tree component #112

feat(components): a simple HTML-based Tree component #112

Conversation

brprice commented Oct 28, 2021 • edited Loading

brprice commented Oct 28, 2021 • edited Loading

dhess commented Oct 28, 2021

brprice commented Nov 15, 2021 • edited Loading

dhess commented Nov 15, 2021 • edited Loading

brprice commented Nov 16, 2021

dhess commented Nov 16, 2021

brprice commented Nov 16, 2021

dhess commented Nov 16, 2021 • edited Loading

dhess left a comment

Choose a reason for hiding this comment

dhess Nov 16, 2021

Choose a reason for hiding this comment

brprice Nov 17, 2021

Choose a reason for hiding this comment

dhess Nov 17, 2021

Choose a reason for hiding this comment

dhess Nov 16, 2021

Choose a reason for hiding this comment

dhess Nov 16, 2021

Choose a reason for hiding this comment

dhess commented Nov 16, 2021 • edited Loading

dhess commented Nov 16, 2021 • edited Loading

brprice commented Nov 17, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

try

dhess commented Nov 24, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

brprice commented Nov 24, 2021

dhess commented Nov 24, 2021

brprice commented Nov 24, 2021

ghost commented Nov 24, 2021

ghost commented Nov 24, 2021

try

brprice commented Oct 28, 2021 •

edited

Loading

brprice commented Oct 28, 2021 •

edited

Loading

brprice commented Nov 15, 2021 •

edited

Loading

dhess commented Nov 15, 2021 •

edited

Loading

dhess commented Nov 16, 2021 •

edited

Loading

dhess commented Nov 16, 2021 •

edited

Loading

dhess commented Nov 16, 2021 •

edited

Loading