feat(Header): use InternalLink for breadcrumb items #913
Conversation
…15-use-link-for-breadcrumbs
src/containers/Header/Header.scss
Outdated
| } | ||
| } | ||
|
|
||
| .g-breadcrumbs { |
There was a problem hiding this comment.
Let's try not to override library styles, but add custom classNames to components and define all stuff there.
There was a problem hiding this comment.
@Raubzeug btw, the solution is tricky enough
https://stackoverflow.com/questions/69883170/is-there-an-eslint-rule-to-prevent-assignment-in-a-single-line-if-statement
There was a problem hiding this comment.
It will be very easy when updated to eslint 9:)
src/containers/Header/Header.tsx
Outdated
| import React from 'react'; | ||
|
|
||
| import {Breadcrumbs} from '@gravity-ui/uikit'; | ||
| import _ from 'lodash'; |
There was a problem hiding this comment.
please import one function you need
src/containers/Header/Header.tsx
Outdated
| clusterName: clusterNameFinal, | ||
| }; | ||
| } | ||
| if (mainPage) rawBreadcrumbs.push(mainPage); |
There was a problem hiding this comment.
lets always use curly braces, like it is done all over the code. Once we update linters, we'll add a rule for this.
src/containers/Tablets/Tablets.tsx
Outdated
| return ( | ||
| <span> | ||
| {row.Type} {row.Leader ? <Text color="secondary">leader</Text> : ''} | ||
| {row.Type} {row.Leader ? '' : <Text color="secondary">follower</Text>} |
There was a problem hiding this comment.
Do you mind if we do it in separate PR?
| const link = nodeId ? getDefaultNodePath(nodeId, query) : undefined; | ||
| const icon = isStorageNode ? <StorageNodeIcon /> : <ComputeNodeIcon />; | ||
| let text = headerKeyset('breadcrumbs.node'); | ||
| if (nodeId) text += ` ${nodeId}`; |
| : headerKeyset('breadcrumbs.pDisk'); | ||
| const link = pDiskId && nodeId ? getPDiskPagePath(pDiskId, nodeId, query) : undefined; | ||
| let text = headerKeyset('breadcrumbs.pDisk'); | ||
| if (pDiskId) text += ` ${pDiskId}`; |
| return rawBreadcrumbs; | ||
| } | ||
| } | ||
| if (!page) return rawBreadcrumbs; |
|
|
||
| return [...rawBreadcrumbs, ...getter(options, query)].map((item) => ({ | ||
| ...item, | ||
| action: () => {}, |
There was a problem hiding this comment.
let's return here raw breadcrumbs without action (cause you already add action in https://github.com/ydb-platform/ydb-embedded-ui/pull/913/files#diff-aa4640b8027696409b37009a150d396684a06d0dd3d7ec6c4abbeb6a74956480R51
There was a problem hiding this comment.
I guess this placement is more appropriate but actually not a big deal
Done
There was a problem hiding this comment.
Actually I agree with you. I meant that adding action should be done in one place :)
This reverts commit d7a7c9e.
src/containers/Header/Header.scss
Outdated
| color: var(--g-color-text-primary); | ||
| border-bottom: 1px solid var(--g-color-line-generic); | ||
|
|
||
| a { |
There was a problem hiding this comment.
Lets not define styles for html-tags, it's bad practice, that may lead to collisions and unwanted side-effects. Please use custom classNames.
src/containers/Header/Header.scss
Outdated
| } | ||
| } | ||
| &__breadcrumbs { | ||
| &_item { |
There was a problem hiding this comment.
Such notation is used for modifiers. For example: breadcrumbs_active or breadcrumbs_warning.
In this case it is an entity. Lets define it like &__breadcrumbs-item next to &__breadcrumbs (not inside it).
src/containers/Header/Header.scss
Outdated
| } | ||
|
|
||
| margin-right: 3px; | ||
| &_current { |
There was a problem hiding this comment.
And this is definitely a modifier!
src/containers/Header/Header.scss
Outdated
|
|
||
| &__icon { | ||
| display: flex; | ||
| &_icon { |
There was a problem hiding this comment.
This is an entity to. Lets define it like &__breadcrumbs-icon.
There was a problem hiding this comment.
In our code, we try to avoid more than one level of nesting (except for modifiers). This means that entities should follow each other and not be enclosed.
NaiSidera
dismissed
artemmufazalov’s stale review
Changes are done, person is on vacation
No description provided.