feat: Display endpoint method and icons for schema and API #195

mallachari · 2020-11-16T03:49:58Z

Changes:

fixes collapsing all descendant children nodes when toggling parent (nested nodes remained open - I've added a simple story for that)
adds toc item icons positioning (we need to have them on both sides)
allows using text labels instead of icons (i.e. for oas operation labels)
splits group items containing to property so they can have both actions handled

~~Note: note sure if we should use paid FA icons here (as in original issue). IIRC elements were supposed to use free version which leaves platform without a choice.~~

mpodlasin · 2020-11-16T14:36:48Z

.storybook/preview-head.html

@@ -0,0 +1,3 @@
+<!-- .storybook/preview-head.html -->
+
+<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/5.15.1/css/all.min.css">


This is interesting to me.

Does it mean in production we are also loading those styles from cdn?

When I used font-awesome icons last time I remember, React version was including the styles by itself (or at the very least they were coming from node_modules package, not cdn).

In production we don't need to do that this way, but icons in storybook here and in elements don't work (at least for me) so I added this which helped. Though I didn't put much attention to that, it could probably be done differently, I just wanted to be able to test the pr

I know the FA situation is still weird.

Currently ui-kit doesn't contain FontAwesome itself but documents it as a dependency to be installed by its users. What @mallachari did here was what we usually do in storybooks to satisfy such deps, in SaaS production we install from node_modules as you said.

One reason you don't want to be installing FA in a library as a regular dep is that many sites already use it (mostly using some CDN), and bundling it would result in duplication and possible version conflicts. We can discuss a better FA strategy on Slack if you have ideas, this is of course by no means perfect.

.storybook/preview-head.html

mpodlasin · 2020-11-16T21:35:59Z

I don't know why, but in my storybook, in nested story, clicking on a topmost button does nothing. I have to click on the arrow icon specifically to make it work.

Interestingly all the other buttons, apart from topmost one, work as expected. 🤔

And it happens only in nested story, other stories work fine.

mallachari · 2020-11-16T22:14:57Z

I don't know why, but in my storybook, in nested story, clicking on a topmost button does nothing. I have to click on the arrow icon specifically to make it work.

Interestingly all the other buttons, apart from topmost one, work as expected.

And it happens only in nested story, other stories work fine.

That is the feature :D
It's group item with link so clicking it unlike others navigates to node and only arrow expands it.

Check out the requirements for Project ToC stoplightio/elements#638

It says:

When generating the TableOfContents for an API, add the API as a group with tags, endpoints, and schemas nested inside of it
Clicking the API should navigate to API overview, and not toggle expand/collapse
Clicking "caret" icon for API should toggle the group expanded or collapsed

This is how it works in following elements PR stoplightio/elements#722 - you can play with it more there

marcelltoth

I don't know why, but in my storybook, in nested story, clicking on a topmost button does nothing. I have to click on the arrow icon specifically to make it work.

Interestingly all the other buttons, apart from topmost one, work as expected.

And it happens only in nested story, other stories work fine.

That is the feature :D
It's group item with link so clicking it unlike others navigates to node and only arrow expands it.

TBH I completely agree with @mpodlasin 's opinion on this. It's horrible user experience. The same user action "clicking an item" that you are used to suddenly "just doesn't work" on some items, without having a clear indicator on why that is the case. Especially true for the API Overview page which is the default page, so trying to expand that node will be a complete no-op for the user as they are already on that page.

Having said that, this is a design issue. The PR delivers what was asked for and is very important to finish the rest of the issues so gets the ✔️ from me. But cc @philsturgeon we'll likely need a followup adjustment here.

I left some comments but none of them are blockers. 🚢 it!

marcelltoth · 2020-11-17T07:27:46Z

src/TableOfContents/index.tsx

+      let childrenToCollapse = {};
+      if (expanded[i]) {
+        const item = contents[i];
+        const children = findDescendantIndices(item.depth ?? 0, i, contents.slice(i + 1));
+        childrenToCollapse = children.reduce((obj, index) => {
+          obj[index] = false;
+          return obj;
+        }, {});
+      }
+      setExpanded(current => {
+        return {
+          ...current,
+          [i]: !current[i],
+          ...childrenToCollapse,
+        };
+      });


There was some reason I moved some of the logic to inside the callback function. Likely it's safer there, but in any case, I'd rather have it at a single place. What do you think of

setExpanded(current => { let childrenToCollapse = {}; if (current[i]) { const item = contents[i]; const children = findDescendantIndices(item.depth ?? 0, i, contents.slice(i + 1)); childrenToCollapse = children.reduce((obj, index) => { obj[index] = false; return obj; }, {}); } return { ...current, [i]: !current[i], ...childrenToCollapse, }; });

And while you're there you could consider

childrenToCollapse = Object.fromEntries(children.map(i => [i, false]));

which is the same as your reduce.

Yup, that would be good, I've changed it.
fromEntries indeed might be better - more readable too.

marcelltoth · 2020-11-17T07:28:39Z

src/TableOfContents/index.tsx

+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [contents, contents.length, expanded]);


Oh this is one of the reasons why.
If you make the above change you'll only depend on contents and can get rid of the eslint inhibit.

I think we still would need that contents.length, just contents would be enough to react on length change, right?

marcelltoth · 2020-11-17T07:30:43Z

src/TableOfContents/index.tsx

  icon?: FAIconProp;
  activeIcon?: FAIconProp;
  iconColor?: string;
+  iconPosition?: 'left' | 'right';
+  textIcon?: string;


It's starting to feel like our abstraction is broken: there's way too many icon props allowing for not too many use-cases.
Not something to block this PR, but next time we touch ToC this will likely need some cleanup.

marcelltoth · 2020-11-17T07:34:23Z

src/TableOfContents/index.tsx

-              className="TableOfContentsItem__icon"
-              icon={['far', isExpanded ? 'chevron-down' : 'chevron-right']}
-            />
+            <div onClick={() => isGroupItem && toggleExpanded()} className="px-2">


IMO a nicer way to express this would be onClick={isGroupItem ? toggleExpanded : undefined}. A tiny bit more efficient as well but mainly more expressive:

If it's a group item then we handle the click like this. Otherwise we don't handle it at all.

Good point. Done

Concerns addressed in comment

mallachari · 2020-11-17T08:34:27Z

I also had these thoughts about that but that's some kind of trade off for not having Overview item. In actual ToC it feels better as clicking it has an action of switching node.

stoplight-bot · 2020-11-17T08:43:27Z

🎉 This PR is included in version 3.0.0-beta.38 🎉

The release is available on:

Your semantic-release bot 📦🚀

mallachari added 4 commits November 16, 2020 00:24

feat: toc group item and icons

863f80c

fix: collapsing nested items

c96f1b0

fix: storybook icons

95b6b25

chore: update fixture

21e34f4

mallachari requested review from a team and removed request for a team November 16, 2020 03:49

mallachari mentioned this pull request Nov 16, 2020

feat: API and Project TOC stoplightio/elements#722

Merged

mallachari self-assigned this Nov 16, 2020

mpodlasin previously requested changes Nov 16, 2020

View reviewed changes

chore: more margin

053ef10

marcelltoth approved these changes Nov 17, 2020

View reviewed changes

chore: cleanup

10eb0d1

mallachari merged commit 3ccebba into beta Nov 17, 2020

mallachari deleted the feat/toc-toggling-and-icons branch November 17, 2020 08:38

stoplight-bot added the released on @beta label Nov 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Display endpoint method and icons for schema and API #195

feat: Display endpoint method and icons for schema and API #195

mallachari commented Nov 16, 2020 •

edited

Loading

mpodlasin Nov 16, 2020

mallachari Nov 16, 2020

marcelltoth Nov 17, 2020

mpodlasin commented Nov 16, 2020 •

edited

Loading

mallachari commented Nov 16, 2020 •

edited

Loading

marcelltoth left a comment

marcelltoth Nov 17, 2020

mallachari Nov 17, 2020

marcelltoth Nov 17, 2020

mallachari Nov 17, 2020

marcelltoth Nov 17, 2020

marcelltoth Nov 17, 2020

mallachari Nov 17, 2020

mallachari commented Nov 17, 2020

stoplight-bot commented Nov 17, 2020

		@@ -0,0 +1,3 @@
		<!-- .storybook/preview-head.html -->

		<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/5.15.1/css/all.min.css">

		// eslint-disable-next-line react-hooks/exhaustive-deps
		}, [contents, contents.length, expanded]);

feat: Display endpoint method and icons for schema and API #195

feat: Display endpoint method and icons for schema and API #195

Conversation

mallachari commented Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpodlasin commented Nov 16, 2020 • edited Loading

mallachari commented Nov 16, 2020 • edited Loading

marcelltoth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mallachari commented Nov 17, 2020

stoplight-bot commented Nov 17, 2020

mallachari commented Nov 16, 2020 •

edited

Loading

mpodlasin commented Nov 16, 2020 •

edited

Loading

mallachari commented Nov 16, 2020 •

edited

Loading