Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Apply various fixes for new Sidebar/Header layout #6806

Merged
merged 3 commits into from
Jan 12, 2024
Merged

Apply various fixes for new Sidebar/Header layout #6806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a78d52
console,account: Apply various fixes
kschiffer Jan 4, 2024
c988bda
console,account: Apply various fixes
kschiffer Jan 4, 2024
686a6e5
console,account: Apply more fixes
kschiffer Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Prevent vague prop types.
react/forbid-prop-types:
- warn
Expand Down
2 changes: 1 addition & 1 deletion pkg/webui/account/containers/clients-table/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<DeleteModalButton
entityId={details.id}
entityName={name}
Expand Down
2 changes: 1 addition & 1 deletion pkg/webui/account/views/landing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


import Footer from '@ttn-lw/containers/footer'

Expand Down
2 changes: 1 addition & 1 deletion pkg/webui/account/views/oauth-client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { Routes, Route, useParams } from 'react-router-dom'
import applicationIcon from '@assets/misc/application.svg'

import { useBreadcrumbs } from '@ttn-lw/components/breadcrumbs/context'
import SideNavigation from '@ttn-lw/components/navigation/side'
import SideNavigation from '@ttn-lw/components/sidebar/side-menu'
import Breadcrumb from '@ttn-lw/components/breadcrumbs/breadcrumb'
import Breadcrumbs from '@ttn-lw/components/breadcrumbs'

Expand Down
9 changes: 5 additions & 4 deletions pkg/webui/components/breadcrumbs/breadcrumb/breadcrumb.styl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,29 +14,30 @@


.container
display: flex

&:not(:last-child):after
material-icon()
nudge('down', 1px)
margin: 0 $cs.xxs
content: 'keyboard_arrow_right'
color: $tc-subtle-gray


.link
one-liner()
text-decoration: none
color: $tc-subtle-gray
min-height: 18px
display: flex
align-items: center
+focus-visible()
text-decoration: underline
color: $tc-deep-gray


.last
one-liner()
display: flex
align-items: center
color: $tc-deep-gray
min-height: 18px
font-weight: $fw.bold
+media-query($bp.s)
&:last-child
Expand Down
6 changes: 3 additions & 3 deletions pkg/webui/components/breadcrumbs/breadcrumb/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,17 @@ const Breadcrumb = ({ className, path, content, isLast }) => {
if (!isLast) {
Component = Link
componentProps = {
className: classnames(className, style.container, style.link),
className: style.link,
to: path,
secondary: true,
}
} else {
Component = 'span'
componentProps = { className: classnames(className, style.container, style.last) }
componentProps = { className: classnames(className, style.last) }
}

return (
<span {...componentProps}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<span className={classnames(className, style.container)}>
<Component {...componentProps}>
{isRawText ? content : <Message content={content} />}
</Component>
Expand Down
5 changes: 1 addition & 4 deletions pkg/webui/components/breadcrumbs/breadcrumbs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import React from 'react'
import ReactDom from 'react-dom'
import classnames from 'classnames'
import { Container } from 'react-grid-system'

import PropTypes from '@ttn-lw/lib/prop-types'

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<Breadcrumbs {...rest} />
</Container>
<Breadcrumbs {...rest} />
</div>,
element,
),
Expand Down
4 changes: 3 additions & 1 deletion pkg/webui/components/breadcrumbs/breadcrumbs.styl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@

.breadcrumbs
color: $tc-subtle-gray
height: $breadcrumbs-bar-height
display: flex
align-items: center
overflow: hidden
height: 1rem

+media-query($bp.s)
overflow: auto
Expand All @@ -30,3 +30,5 @@
&-container
box-sizing: border-box
height: $breadcrumbs-bar-height
display: flex
align-items: center
173 changes: 60 additions & 113 deletions pkg/webui/components/button/button.styl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,69 +13,64 @@
// limitations under the License.

.button
// 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.
Comment on lines +16 to +19
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

border-radius: $br.l
outline: 0
cursor: pointer
justify-content: center
align-items: center
gap: $cs.xxs
height: 2.5rem
text-decoration: none
padding: 0 $cs.m
white-space: nowrap

.arrow-icon
transform: rotate(0deg)
transition: transform .2s

.arrow-icon-expanded
transform: rotate(180deg)
transition: transform .2s

&.primary, &.secondary, &.naked
position: relative
display: inline-flex
transition: 80ms background ease-in-out, 80ms color ease-in-out, 80ms border-color ease-in-out, 80ms box-shadow ease-in-out
border-radius: $br.m
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

outline: 0
cursor: pointer
justify-content: center
align-items: center
gap: $cs.xxs
height: 2.5rem
text-decoration: none
padding: 0 $cs.m
white-space: nowrap
box-sizing: border-box

.icon
margin-left: - $cs.xxs

&:only-child
margin-right: - $cs.xxs
Comment on lines +38 to +42
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is applied to make the outer padding based on the visual bounding box of the icon, rather than it's container.


.expand-icon
color: $c.grey-500
font-size: 1.285rem
margin-right: - $cs.xxs

&.only-icon
gap: 0
padding: 0 $cs.s

&.primary
transition: 0.2s all ease-in-out
color: white
background-color: $c-active-blue

&.disabled
&:disabled
background-color: $c-divider-dark

&:not(.disabled)
&:not(:disabled)
&:hover
transition: 0.2s all ease-in-out
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This transition prop was set at least a dozen times in various levels of the button. I don't get why, since it was already set on the base class. Let's avoid these kind of spray-can solutions.

background-color: hoverize($c-active-blue)

+focus-visible()
background-color: hoverize($c-active-blue)

&:active
transition: 0.2s all ease-in-out
background-color: activize($c-active-blue)

&.grey
background-color: $c.grey-500

&:not(.disabled)
&:hover
background-color: hoverize($c.grey-500)

+focus-visible()
background-color: hoverize($c.grey-500)

&:active
background-color: activize($c.grey-500)

&.disabled
background-color: transparentify($c.grey-500, #fff, .25)

&.warning
background-color: $c-warning

&:not(.disabled)
&:not(:disabled)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the semantically correct :disabled pseudo selector, to avoid ambiguity and also ensure that the button is actually disabled on the browser level and not just as a styling.

&:hover
background-color: hoverize($c-warning)

Expand All @@ -88,7 +83,7 @@
&.danger
background-color: $c-error

&:not(.disabled)
&:not(:disabled)
&:hover
background-color: hoverize($c-error)

Expand All @@ -98,112 +93,67 @@
&:active
background-color: activize($c-error)

&.disabled
&:disabled
background-color: transparentify($c-error, #fff, .25)

&.secondary
transition: 0.2s all ease-in-out
color: $tc-deep-gray
background-color: white
box-shadow: 0px 2px 2px 0px rgba(0 0 0 4.5%), inset 0px 0px 0px 1px white
background: linear-gradient(to bottom, white, white) padding-box, linear-gradient(to bottom, #E8E7EC, #D7D7D7) border-box
border: 1px solid transparent


&:hover
transition: 0.2s all ease-in-out
background: linear-gradient(to bottom, $c-focus, $c-focus) padding-box, linear-gradient(to bottom, #E8E7EC, #D7D7D7) border-box

+focus-visible()
background: linear-gradient(to bottom, $c-focus, $c-focus) padding-box, linear-gradient(to bottom, #E8E7EC, #D7D7D7) border-box

&:active
transition: 0.2s all ease-in-out
background: linear-gradient(to bottom, $c-focus, $c-focus) padding-box, linear-gradient(to bottom, #E8E7EC, #D7D7D7) border-box
box-shadow: none

&.naked
transition: 0.2s all ease-in-out
border: none
background-color: none
box-shadow: unset
color: $tc-subtle-gray

&.disabled
&:disabled
color: transparentify($tc-subtle-gray, #fff, .25) !important

&:not(.disabled)
&:not(:disabled)
&:hover
background-color: $c-backdrop
background-color: rgba(0,0,0,.04)
color: $c.grey-700

+focus-visible()
background-color: $c-backdrop
background-color: rgba(0,0,0,.04)
color: $c.grey-700

&:active
transition-duration: $ad.xs
background-color: $c-backdrop

&.warning
color: $tc-warning

&.danger
color: $tc-error

&:not(.primary):not(.naked)
border-dark()
border: 1px solid #d3d3d3
box-sizing: border-box
background-color: #fff
color: $tc-deep-gray

&:not(:active)
box-shadow: 0px 1px 1px #efefef, inset 0 1px 0 white

&.disabled
border-color: transparentify(#d3d3d3, #fff, .1)

&:not(.busy)
color: transparentify($tc-deep-gray, #fff, .25)

&:not(.disabled)
&:hover
background-color: #fafafa
&.only-icon
width: 2.5rem
height: 2.5rem

+focus-visible()
border: 1px solid $c-active-blue
background-color: #fafafa

&:active
transition-duration: $ad.xs
background-color: #f6f6f6
&.small
width: 2rem
height: 2rem
border-radius: $br.s

&.warning
border-color: $tc-warning
color: $tc-warning
color: $c.orange-warning-500

&:not(.disabled)
&:hover
color: hoverize($tc-warning)
&:hover
color: $c.orange-warning-600

&.danger
border-color: $tc-error
color: $tc-error
color: $c.red-error-500

&:not(.disabled)
&:hover
color: hoverize($tc-error)

&.only-icon
width: 2.5rem
padding: 0 $cs.xs 0 0.73rem

&.with-dropdown
padding: 0 $cs.xxs 0 $cs.xs
&:hover
color: $c.red-error-600

&.with-dropdown&.only-icon
width: fit-content
.icon
margin-left: 0

&.with-icon
line-height: 1.3
Expand All @@ -222,7 +172,7 @@
&.error
animation: shake .15s 4 linear

&.disabled
&:disabled
cursor: default

&.busy:not(.naked),
Expand All @@ -244,9 +194,6 @@
left: - $cs.s
right: - $cs.s

.icon
margin-left: - $cs.xxs

.button-group
display: flex
flex-direction: column
Expand Down
Loading
Loading