CardView: Implement Toolbar #28693
CardView: Implement Toolbar #28693
Conversation
| const propsToUpdate = { ...props }; | ||
| delete propsToUpdate.items; |
There was a problem hiding this comment.
Let's use more modern syntax with destructing
| const propsToUpdate = { ...props }; | |
| delete propsToUpdate.items; | |
| const {items, ...propsToUpdate} = props; |
| public addDefaultItem( | ||
| item: MaybeSubscribable<PredefinedToolbarItem>, | ||
| ): void { | ||
| toSubscribable(item).subscribe((item) => { |
There was a problem hiding this comment.
Please add unsubscribing for this subscription to removeDefaultItem method
| 'revertButton', | ||
| 'editButton', | ||
| 'columnsChooserButton', | ||
| 'searchPanel', |
There was a problem hiding this comment.
Minor
Let's make this array empty for now. We'll add buttons later one-by-one when we'll implement corresponding module
|
|
||
| describe('when visibleConfig = undefined and there are no items', () => { | ||
| it('should be equal to true', () => { | ||
| expect(isVisible(undefined, [{ name: 'toolbarItem1' }, { name: 'toolbarItem2' }])).toBe(true); |
There was a problem hiding this comment.
Minor
Let's format this line into multiline
|
|
||
| export type PredefinedToolbarItem = ToolbarItem & { name: DefaultToolbarItem }; | ||
| export type ToolbarItems = (ToolbarItem | DefaultToolbarItem)[]; | ||
| export type ToolbarVisible = boolean | undefined; |
There was a problem hiding this comment.
Let's inline this type
It's hard to understand what it means until you open its definition
| name?: string; | ||
| } | ||
|
|
||
| function normalizeToolbarItem( |
There was a problem hiding this comment.
This utilities work with toolbar, so they should be in toolbar folder
| }; | ||
|
|
||
| describe('render', () => { | ||
| it('empty toolbar', () => { |
There was a problem hiding this comment.
Let's follow the pattern describe('some entity in code') -> describe('some case') -> it('should ...')
there's no entity "render" in code, so let's rewrite it as
describe("Toolbar") -> describe("when it is empty") -> describe("it should be rendered correctly")`
And if it possible, instead of "should be rendered correctly" let's use something more specific, like "should be rendered to empty div" or "should not be rendered"
|
|
||
| describe('normalizeToolbarItems', () => { | ||
| describe('when only default items are specified', () => { | ||
| it('should return correct items', () => { |
There was a problem hiding this comment.
Looks like it's too much unspecific test names. It is not quite understandable what you mean by "should return correct items". It is okay to use it sometimes, but looks like here we can write something more specific
No description provided.