feat: add DefinitionList #143
Conversation
|
Preview is ready. |
| @@ -0,0 +1,102 @@ | |||
| @use '../variables'; | |||
| @use '@gravity-ui/uikit/styles/mixins'; | |||
There was a problem hiding this comment.
This works without ~ correctly?
There was a problem hiding this comment.
This syntax is used all over in the repo. No problem.
| gap: 4px; | ||
|
|
||
| & + & { | ||
| margin-top: 16px; |
There was a problem hiding this comment.
| margin-top: 16px; | |
| margin-top: var(--g-spacing-4); |
There was a problem hiding this comment.
What is the purpose of this file?
There was a problem hiding this comment.
Totally no purpose. Artefact crawled into PR.
Raubzeug
force-pushed
the
add-definition-list
branch
from
894bfc5
to
6e70826
Compare
Raubzeug
force-pushed
the
add-definition-list
branch
from
5893369
to
c53531b
Compare
Raubzeug
force-pushed
the
add-definition-list
branch
from
c53531b
to
017aa42
Compare
|
Copy button not consistent. Let's fix it at the top right corner 
|
|
Copy button not accessible from keyboard untill shown |
|
|
||
| #{$block}__copy-button { | ||
| inset-block-start: 0; | ||
| background-color: var(--g-color-base-background); |
There was a problem hiding this comment.
There is CSS API of Button to customize styling
There was a problem hiding this comment.
I've updated it, the new version is on Files changed tab.
| color: var(--g-color-text-secondary); | ||
|
|
||
| &:hover { | ||
| color: var(--g-color-text-primary); | ||
| } |
There was a problem hiding this comment.
Why default Button's views not work for this component?
There was a problem hiding this comment.
I don't know why I decided to do it...
There was a problem hiding this comment.
I've updated it, the new version is on Files changed tab.
| return ( | ||
| <dl className={b({responsive}, className)} role="list"> | ||
| {items.map(({name, href, content, title, copyText, note, key, multilineName}) => { | ||
| const term = href ? <Link href={href}>{name}</Link> : name; |
There was a problem hiding this comment.
Maybe instead of having href prop we allow name to be ReactNode to render whatever user want?
There was a problem hiding this comment.
Like it! But in this case key property should be required.
There was a problem hiding this comment.
We can generate them by ourselves
|
|
||
| export interface DefinitionListItem { | ||
| name: React.ReactNode; | ||
| key: string | number; |
| name: React.ReactNode; | ||
| key: string | number; | ||
| content?: React.ReactNode; | ||
| title?: string; |
There was a problem hiding this comment.
Maybe split into 2 props, for name and for content?
| } | ||
|
|
||
| if (typeof note === 'object') { | ||
| noteElement = <HelpPopover className={popoverClassName} {...note} />; |
There was a problem hiding this comment.
Let's also set default placement here
There was a problem hiding this comment.
Why do you think we need it? <Popover> under the hood has default placement and it seems it is enough.
There was a problem hiding this comment.
Then why we set placement when note is a string?
| noteElement = ( | ||
| <HelpPopover | ||
| className={popoverClassName} | ||
| htmlContent={note} |
There was a problem hiding this comment.
Let's use normal content props, not html
| | key | `String \| Number` | true | | Item key | | ||
| | content | `ReactNode` | | | Definition | | ||
| | title | `String` | | | Title for definition. If not set, `content` value will be used | | ||
| | copyText | `String` | | | If set, it will be shown icon for copy this text | |
No description provided.