Apply various fixes for new Sidebar/Header layout #6806

kschiffer · 2024-01-04T08:03:43Z

Summary

This PR adds various functionality, implementation and styling fixes to the Sidebar/Header scaffold.

Changes

Fix styling issues of the sidebar
Fix styling issues of the header
Improve dropdown component
Improve button component
Improve animations

Notes for Reviewers

I've left many comments on the diff to explain various changes. @ryaplots @PavelJankoski please observe the changes corresponding to your components.

Checklist

Scope: The referenced issue is addressed, there are no unrelated changes.
Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
Documentation: Relevant documentation is added or updated.
The steps/process to test this feature are clearly explained including testing for regressions.
Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

kschiffer · 2024-01-04T08:06:04Z

config/eslintrc.yaml

@@ -141,6 +141,8 @@ rules:
  react/forbid-foreign-prop-types: off
  # Prevent undefined components.
  react/jsx-no-undef: warn
+  # Allow arrow functions as props
+  react/jsx-no-bind: off


I believe it's not common anymore to completely disallow arrow functions as props. There are situations where it's fine to use arrow functions, mainly for very simple one-off functions. We should still avoid them for complex functions that should rather be memoized.

kschiffer · 2024-01-04T08:07:27Z

pkg/webui/account/containers/clients-table/index.js

@@ -151,7 +151,7 @@ const ClientsTable = () => {
        }),
        render: details => (
          <ButtonGroup align="end">
-            <Button message={sharedMessages.restore} onClick={details.restore} />
+            <Button message={sharedMessages.restore} onClick={details.restore} secondary />


This change has to do with a change of how the button works. I believe it's best to always explicitly pass the button style (primary, secondary, naked) and apply no styling at all when no style prop is set, instead of relying on unstyled or raw props.

kschiffer · 2024-01-04T08:08:07Z

pkg/webui/account/views/landing/index.js

@@ -18,7 +18,7 @@ import classnames from 'classnames'

 import authRoutes from '@account/constants/auth-routes'

-import sidebarStyle from '@ttn-lw/components/navigation/side/side.styl'
+import sidebarStyle from '@ttn-lw/components/sidebar/side-menu/side.styl'


I've moved the navigation/side into the sidebar component. I believe this is the only place we should ever use this navigation list in. There are currently some areas where we use it outside of the sidebar (I believe in the Account app), but we will address this later.

Please, also update the path in the side-menu story (pkg/webui/console/containers/side-bar/story.js)

kschiffer · 2024-01-04T08:09:20Z

pkg/webui/components/button/button.styl

+  // Without secondary or primary style,
+  // the button should not apply any styling.
+  // This is to cater for special situations
+  // where the button is used as a container.


See my other comment, this way of styling also avoids a lot of repetition and style overlaps in the button component.

kschiffer · 2024-01-04T08:09:59Z

pkg/webui/components/button/button.styl

+    position: relative
+    display: inline-flex
+    transition: 80ms all ease-in-out
+    border-radius: $br.m


Per design, the border radius for buttons is 7px (not 10.5).

kschiffer · 2024-01-04T08:25:23Z

pkg/webui/components/profile-dropdown/index.js

  const { brandLogo, className, children, profilePicture, ...rest } = props

-  const handleClickOutside = useCallback(e => {


There was quite some logic duplication in here with regards to show/hiding the dropdown. I've refactored this to use the existing logic of the dropdown component.

kschiffer · 2024-01-04T08:26:02Z

pkg/webui/components/sidebar/dedicated-entity/dedicated-entity.styl

+  position: relative
+  height: 2.5rem
+
+  &-curtain
+    position: absolute
+    overflow: hidden
+    border-radius: $br.l
+    width: 2.5rem
+    transition: 0.3s 0s all cubic-bezier(0.770, 0.000, 0.175, 1.000)
+    top: 0
+    left: 0
+    z-index: $zi.slight
+
+    &:hover
+      transition: 0.3s 0.2s all cubic-bezier(0.770, 0.000, 0.175, 1.000)
+      width: 100%
+
+      // Reveal the button label.
+      button span:last-child
+        opacity: 1


This fixes the jagged motion in the transition.

kschiffer · 2024-01-04T08:26:31Z

pkg/webui/components/sidebar/dedicated-entity/index.js

@@ -13,7 +13,7 @@
 // limitations under the License.

 import React from 'react'
-import { NavLink } from 'react-router-dom'
+import { Link } from 'react-router-dom'


No need for NavLink here

kschiffer · 2024-01-04T08:30:09Z

pkg/webui/components/sidebar/side-menu/item/link.js

@@ -43,6 +44,13 @@ const MenuLink = ({ icon, title, path, onClick, exact, disabled }) => {
    <NavLink to={path} className={className} end={exact} onClick={onClick}>
      {icon && <Icon icon={icon} className={classnames(style.icon)} />}{' '}
      {!isMinimized && <Message content={title} />}
+      {isMinimized && (


The regular items should also show the flyout on hover, when the sidebar is collapsed.

kschiffer · 2024-01-04T08:35:25Z

pkg/webui/console/views/app/app.styl

@@ -12,18 +12,21 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+.container


Instead of sticking the sidebar to the top, I've adjusted the layout to stay 100% always and then making the main scrollable instead. This makes the layout simpler and avoids weird edge cases with sticky/fixed.

ryaplots

The header is now broken in account:

Check the render method of `Header`.
Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.

pkg/webui/components/sidebar/switcher/index.js

pkg/webui/console/containers/side-bar/navigation/app-list-side-navigation.js

ryaplots · 2024-01-05T11:36:47Z

pkg/webui/components/sidebar/side-footer/side-footer.styl

-  &-support-dropdown
-    right: -79px !important
-
-  &-dropdown
-    width: 18.2rem


Right now the footer dropdowns do not take up the whole width of the sidebar, is that how it's supposed to be?

I'll fix this.

pkg/webui/components/dropdown/dropdown.styl

ryaplots · 2024-01-05T11:46:02Z

pkg/webui/components/sidebar/side-menu/item/collapsible.js

After moving the navigation and separating this, when selecting uplink and then minimizing, it is not not possible to see which section is active. I tried to quickly understand why, but nothing major seemed to have changed.

ryaplots · 2024-01-05T12:27:11Z

pkg/webui/account/views/landing/index.js

@@ -18,7 +18,7 @@ import classnames from 'classnames'

 import authRoutes from '@account/constants/auth-routes'

-import sidebarStyle from '@ttn-lw/components/navigation/side/side.styl'
+import sidebarStyle from '@ttn-lw/components/sidebar/side-menu/side.styl'


Please, also update the path in the side-menu story (pkg/webui/console/containers/side-bar/story.js)

kschiffer · 2024-01-11T07:38:21Z

pkg/webui/components/breadcrumbs/breadcrumb/index.js

  }

  return (
-    <span {...componentProps}>


Assigning the component props to the span would result in secondary being set and interpreted as HTML attribute.

kschiffer · 2024-01-11T07:39:19Z

pkg/webui/components/breadcrumbs/breadcrumbs.js

@@ -52,9 +51,7 @@ const PortalledBreadcrumbs = ({ className, ...rest }) => {
      nodes.push(
        ReactDom.createPortal(
          <div className={classnames(className, style.breadcrumbsContainer)}>
-            <Container>


In the earlier layout, the breadcrumb container was embedded into the grid layout. This is now not the case anymore and it can be rendered as it. Otherwise it will render an additional padding that is not according to the screen design.

kschiffer · 2024-01-11T07:40:53Z

pkg/webui/components/button/button.styl

  reset-button()
-  position: relative
-  display: inline-flex
-  transition: 0.2s all ease-in-out


Setting a transition on all will lead to unwanted transitions on properties such as margin, padding, font-size etc.

kschiffer · 2024-01-11T07:58:26Z

pkg/webui/components/button/index.js


  const dataProps = useMemo(() => filterDataProps(rest), [rest])

-  const handleClickOutside = useCallback(


I've outsourced the entire toggling logic of the dropdown into it's own component <Dropdown.Attached attachedRef={refOfButton} />

Reasons:

Recurring logic that would otherwise need to be rewritten over and over (especially error prone due to the many edge-cases of dropdown toggling [trigger method, outside clicking, toggling, positioning)

To easily enable dropdowns that are not rendered within the triggering button

kschiffer · 2024-01-11T08:07:17Z

pkg/webui/components/button/index.js

-            />
-          )}
-          <Dropdown className={classnames(dropdownClassName)} open={expanded}>
-            {dropdownItems}


Notice that this previous implementation leads to <button />'s being rendered within a <button /> which is not HTML compliant and results in a warning and various accessibility issues.

kschiffer · 2024-01-11T09:17:16Z

pkg/webui/console/containers/side-bar/index.js

-
-const Sidebar = ({ isDrawerOpen }) => {
-  const viewportWidth = getViewportWidth()
-  const isMobile = viewportWidth <= LAYOUT.BREAKPOINTS.M


I think we were talking about this before but the issue with a solution is that this only calculates once during render. However, this would not update if the screen size changes in between renders. For example, if you render in mobile screen size and then resize the browser window to desktop, the sidebar will still think it is isMobile until it renders again.

Generally speaking, using such props like isMobile is typically unnecessary because most of the things that such a prop is used for can be solved by using media-queries instead. In this particular case, I've decided to leave it though since it enables to render tooltip dropdowns only when necessary (instead of hiding them via media-queries but still running mouse-in/out hooks).

kschiffer · 2024-01-11T09:19:22Z

pkg/webui/console/containers/side-bar/index.js

-    'd-flex pos-fixed direction-column j-between gap-cs-m bg-tts-primary-050',
+    'd-flex direction-column j-between gap-cs-m bg-tts-primary-050',
    {
      [style.sidebarMinimized]: isMinimized,
-      [style.sidebarOpen]: isMobile && isDrawerOpen,
-      'p-cs-s': !isMinimized,
-      'p-vert-cs-m': isMinimized,
+      [style.sidebarOpen]: isDrawerOpen,
+      'p-cs-m': !isMinimized,
+      'p-vert-cs-s': isMinimized,
      'p-sides-cs-xs': isMinimized,
    },
  )



This is still not optimal and quite confusing. All of this should rather be just done using plain CSS and media queries.

kschiffer · 2024-01-11T09:20:06Z

pkg/webui/console/containers/side-bar/index.js

+          <SideFooter />
+        </SidebarContext.Provider>
+      </div>
+      <div className={style.sidebarBackdrop} onClick={onDrawerCloseClick} />


Note that the backdrop was missing in the earlier implementation. There was only a box-shadow, which however was not according to the screen design.

kschiffer · 2024-01-11T09:22:42Z

pkg/webui/console/containers/side-bar/navigation/app-side-navigation.js

            className="mt-cs-xs mb-cs-l"
-            path={`/applications`}
+            path={`/applications/${appId}`}


This had to be refactored throughout because for some reason after the refactor, the link would be rendered as applications/applications/app-id.
Must've something to do with how react router determines the current route by calculating all <Route />'s up the render hierarchy.

kschiffer · 2024-01-11T09:28:35Z

pkg/webui/styles/mixins.styl

@@ -361,7 +361,7 @@ transition-color()

 sidebar-transition($properties)
  transition-timing-function: cubic-bezier(.18,.71,.3,.99)
-  transition-duration: 330ms
+  transition-duration: 0ms


This needs to be streamlined and fixed eventually.

kschiffer · 2024-01-12T05:32:56Z

pkg/webui/console/containers/side-bar/index.js

-
-const Sidebar = ({ isDrawerOpen }) => {
-  const viewportWidth = getViewportWidth()
-  const isMobile = viewportWidth <= LAYOUT.BREAKPOINTS.M


Generally speaking, using such props like isMobile is typically unnecessary because most of the things that such a prop is used for can be solved by using media-queries instead. In this particular case, I've decided to leave it though since it enables to render tooltip dropdowns only when necessary (instead of hiding them via media-queries but still running mouse-in/out hooks).

kschiffer · 2024-01-12T05:34:18Z

pkg/webui/console/containers/side-bar/navigation/app-side-navigation.js

+  if (!app) {
+    return null
+  }


Before this, there were proptype warnings in the console for initial renders, when the app was not yet available.

console,account: Apply various fixes

4a78d52

kschiffer added c/console This is related to the Console ui/web This is related to a web interface labels Jan 4, 2024

kschiffer self-assigned this Jan 4, 2024

github-actions bot removed the c/console This is related to the Console label Jan 4, 2024

kschiffer commented Jan 4, 2024

View reviewed changes

ryaplots requested changes Jan 5, 2024

View reviewed changes

console,account: Apply various fixes

c988bda

kschiffer commented Jan 11, 2024

View reviewed changes

kschiffer force-pushed the fix/new-sidebar-fixes branch from 513c7bc to 26e507c Compare January 11, 2024 09:28

console,account: Apply more fixes

686a6e5

kschiffer force-pushed the fix/new-sidebar-fixes branch from 26e507c to 686a6e5 Compare January 12, 2024 05:24

kschiffer commented Jan 12, 2024

View reviewed changes

kschiffer marked this pull request as ready for review January 12, 2024 05:37

kschiffer requested a review from a team as a code owner January 12, 2024 05:37

kschiffer requested review from mjamescompton and removed request for a team January 12, 2024 05:37

kschiffer merged commit f5f7ca3 into feature/new-sidebar Jan 12, 2024
5 of 12 checks passed

kschiffer deleted the fix/new-sidebar-fixes branch January 12, 2024 05:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply various fixes for new Sidebar/Header layout #6806

Apply various fixes for new Sidebar/Header layout #6806

kschiffer commented Jan 4, 2024 •

edited

Loading

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

ryaplots Jan 5, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

kschiffer Jan 4, 2024

ryaplots left a comment •

edited

Loading

ryaplots Jan 5, 2024

kschiffer Jan 8, 2024

ryaplots Jan 5, 2024

ryaplots Jan 5, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 12, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 11, 2024

kschiffer Jan 12, 2024

kschiffer Jan 12, 2024

		const { brandLogo, className, children, profilePicture, ...rest } = props

		const handleClickOutside = useCallback(e => {


		const dataProps = useMemo(() => filterDataProps(rest), [rest])

		const handleClickOutside = useCallback(

Apply various fixes for new Sidebar/Header layout #6806

Apply various fixes for new Sidebar/Header layout #6806

Conversation

kschiffer commented Jan 4, 2024 • edited Loading

Summary

Changes

Notes for Reviewers

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryaplots left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer commented Jan 4, 2024 •

edited

Loading

ryaplots left a comment •

edited

Loading