Unit Navigation Tabs have ugly rounding/borders

[bradenmacdonald]

The unit tabs used to quickly navigate between units in a subsection don't follow standard UX practice of merging into a "button group" where only the outer borders are rounded.

Current UI:

Expected UI:

• only the outer borders are rounded
• Adjoining (inner) borders are merged to a single border (1px) instead of double border (2px)

This may be more of a Paragon issue - it seems to lack an appropriate Tab component that looks like this. Or maybe we should just be using the Paragon Tab component. See https://paragon-openedx.netlify.app/components/tabs/#pills-usage

[brian-smith-tcril]

This may be more of a Paragon issue

It doesn't seem like this specific issue is a Paragon one. The tabs component even supports button group usage with variant="button-group"

It looks like these are being rendered by:

• https://github.com/openedx/frontend-app-authoring/blob/master/src/course-unit/course-sequence/sequence-navigation/SequenceNavigationTabs.jsx
• https://github.com/openedx/frontend-app-authoring/blob/master/src/course-unit/course-sequence/sequence-navigation/SequenceNavigation.jsx
• https://github.com/openedx/frontend-app-authoring/blob/master/src/course-unit/course-sequence/sequence-navigation/UnitButton.tsx
• https://github.com/openedx/frontend-app-authoring/blob/master/src/course-unit/course-sequence/sequence-navigation/SequenceNavigationDropdown.jsx

none of those use the Paragon ButtonGroup or Tab components at all, just Button and Dropdown.

I tried a quick change locally to get a sense of what it might take to start using a ButtonGroup here. I had to remove some bounding box checking and hardcode a few things but got

from

diff --git a/src/course-unit/course-sequence/hooks.js b/src/course-unit/course-sequence/hooks.js
index 28035e1a..2dbff4f6 100644
--- a/src/course-unit/course-sequence/hooks.js
+++ b/src/course-unit/course-sequence/hooks.js
@@ -80,7 +80,10 @@ export function useIndexOfLastVisibleChild() {
   const windowSize = useWindowSize();
 
   useLayoutEffect(() => {
-    const containingRect = containerElementRef.current.getBoundingClientRect();
+    const containingRect = containerElementRef?.current?.getBoundingClientRect();
+    if (!containingRect) {
+      return;
+    }
 
     // No-op if the width is unchanged.
     // (Assumes tabs themselves don't change count or width).
diff --git a/src/course-unit/course-sequence/sequence-navigation/SequenceNavigation.jsx b/src/course-unit/course-sequence/sequence-navigation/SequenceNavigation.jsx
index fcadcd9c..69bc3889 100644
--- a/src/course-unit/course-sequence/sequence-navigation/SequenceNavigation.jsx
+++ b/src/course-unit/course-sequence/sequence-navigation/SequenceNavigation.jsx
@@ -5,7 +5,9 @@ import classNames from 'classnames';
 import {
   useIntl, isRtl, getLocale,
 } from '@edx/frontend-platform/i18n';
-import { Button, useWindowSize, breakpoints } from '@openedx/paragon';
+import {
+  Button, ButtonGroup, useWindowSize, breakpoints,
+} from '@openedx/paragon';
 import {
   ChevronLeft as ChevronLeftIcon,
   ChevronRight as ChevronRightIcon,
@@ -98,9 +100,11 @@ const SequenceNavigation = ({
       className={classNames('sequence-navigation d-flex', className)}
       style={{ width: shouldDisplayNotificationTriggerInSequence ? '90%' : null }}
     >
-      {renderPreviousButton()}
-      {renderUnitButtons()}
-      {renderNextButton()}
+      <ButtonGroup>
+        {renderPreviousButton()}
+        {renderUnitButtons()}
+        {renderNextButton()}
+      </ButtonGroup>
     </nav>
   );
 };
diff --git a/src/course-unit/course-sequence/sequence-navigation/SequenceNavigationTabs.jsx b/src/course-unit/course-sequence/sequence-navigation/SequenceNavigationTabs.jsx
index 0307c2f5..a423ef91 100644
--- a/src/course-unit/course-sequence/sequence-navigation/SequenceNavigationTabs.jsx
+++ b/src/course-unit/course-sequence/sequence-navigation/SequenceNavigationTabs.jsx
@@ -28,7 +28,7 @@ const SequenceNavigationTabs = ({
     containerRef,
     invisibleStyle,
   ] = useIndexOfLastVisibleChild();
-  const shouldDisplayDropdown = indexOfLastVisibleChild === -1;
+  const shouldDisplayDropdown = false;
 
   const handleAddNewSequenceUnit = () => {
     dispatch(updateQueryPendingStatus(true));
@@ -47,41 +47,34 @@ const SequenceNavigationTabs = ({
   };
 
   return (
-    <div className="sequence-navigation-tabs-wrapper">
-      <div className="sequence-navigation-tabs-container d-flex" ref={containerRef}>
-        <div
-          className="sequence-navigation-tabs d-flex flex-grow-1"
-          style={shouldDisplayDropdown ? invisibleStyle : null}
-        >
-          {unitIds.map((buttonUnitId) => (
-            <UnitButton
-              key={buttonUnitId}
-              unitId={buttonUnitId}
-              isActive={unitId === buttonUnitId}
-            />
-          ))}
-          {sequenceChildAddable && (
-          <Button
-            className="sequence-navigation-tabs-action-btn"
-            variant="outline-primary"
-            iconBefore={PlusIcon}
-            onClick={handleAddNewSequenceUnit}
-          >
-            {intl.formatMessage(messages.newUnitBtnText)}
-          </Button>
-          )}
-          {showPasteUnit && (
-            <Button
-              className="sequence-navigation-tabs-action-btn"
-              variant="outline-primary"
-              iconBefore={ContentPasteGoIcon}
-              onClick={handlePasteNewSequenceUnit}
-            >
-              {intl.formatMessage(messages.pasteAsNewUnitLink)}
-            </Button>
-          )}
-        </div>
-      </div>
+    <>
+      {unitIds.map((buttonUnitId) => (
+        <UnitButton
+          key={buttonUnitId}
+          unitId={buttonUnitId}
+          isActive={unitId === buttonUnitId}
+        />
+      ))}
+      {sequenceChildAddable && (
+      <Button
+        className="sequence-navigation-tabs-action-btn"
+        variant="outline-primary"
+        iconBefore={PlusIcon}
+        onClick={handleAddNewSequenceUnit}
+      >
+        {intl.formatMessage(messages.newUnitBtnText)}
+      </Button>
+      )}
+      {showPasteUnit && (
+      <Button
+        className="sequence-navigation-tabs-action-btn"
+        variant="outline-primary"
+        iconBefore={ContentPasteGoIcon}
+        onClick={handlePasteNewSequenceUnit}
+      >
+        {intl.formatMessage(messages.pasteAsNewUnitLink)}
+      </Button>
+      )}
       {shouldDisplayDropdown && (
         <SequenceNavigationDropdown
           unitId={unitId}
@@ -91,7 +84,7 @@ const SequenceNavigationTabs = ({
           showPasteUnit={showPasteUnit}
         />
       )}
-    </div>
+    </>
   );
 };