Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Toolbar): add custom kind of ToolbarItem #1558

Merged
merged 11 commits into from
Jul 2, 2019

Conversation

miroslavstastny
Copy link
Member

@miroslavstastny miroslavstastny commented Jun 27, 2019

custom Toolbar Item kind allows to insert any content into a toolbar item - either non-focusable (Text) or focusable (Button):

<Toolbar
  items={[
    {
      key: 'custom-text',
      kind: 'custom',
      content: <Text content="text" />,
    },
    {
      key: 'custom-button',
      kind: 'custom',
      fitted: 'horizontally',
      content: <Button content="button" />,
    },
  ]}
/>

image

fitted prop

This custom item adds padding by default. This can be customised by specifying fitted prop, which can be:

  • false (default)
  • true - removes all padding
  • 'horizontally' - removes left and right padding
  • 'vertically' - removes top and bottom padding

focused prop

A custom item can be focused.

image

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #1558 into master will decrease coverage by 0.05%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1558      +/-   ##
=========================================
- Coverage   71.56%   71.5%   -0.06%     
=========================================
  Files         842     844       +2     
  Lines        6862    6890      +28     
  Branches     1947    1960      +13     
=========================================
+ Hits         4911    4927      +16     
- Misses       1945    1957      +12     
  Partials        6       6
Impacted Files Coverage Δ
packages/react/src/index.ts 100% <ø> (ø) ⬆️
...hemes/teams/components/Toolbar/toolbarVariables.ts 50% <ø> (ø) ⬆️
packages/react/src/components/Toolbar/Toolbar.tsx 90.47% <100%> (+1%) ⬆️
...react/src/components/Toolbar/ToolbarCustomItem.tsx 100% <100%> (ø)
...eams/components/Toolbar/toolbarCustomItemStyles.ts 7.69% <7.69%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee412c7...d2638b8. Read the comment docs.

import { defaultBehavior } from '../../lib/accessibility'

export interface ToolbarCustomItemProps
extends UIComponentProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to restrict the prop types and remove onClick? onClick should be put on the content element instead as the custom item is not focusable.

Copy link
Contributor

Choose a reason for hiding this comment

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

probably, I am missing something - but currently not sure how aforementioned statement can coexist with focusable prop :)

  /** A custom item can be focused. */
  focusable?: boolean

@miroslavstastny, @jurokapsiar, could you, please, suggest which part I am missing?

Copy link
Member

Choose a reason for hiding this comment

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

Per offline discussion: it should be focusable conditionally, but it can't be actionable (onClick: never). We can consider to change this later based on provided use cases.

@layershifter layershifter changed the title feat(Toolbar) - add custom kind of item feat(Toolbar): add custom kind of ToolbarItem Jul 1, 2019
> {
static displayName = 'ToolbarCustomItem'

static className = 'ui-toolbar__customitem'
Copy link
Contributor

Choose a reason for hiding this comment

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

ui-toolbar__custom-item?

Copy link
Member

Choose a reason for hiding this comment

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

We have a UT for this one:

test(`is a static equal to "${info.componentClassName}"`, () => {

It expects to have: ui-toolbar__customitem

display: 'flex',
alignItems: 'center',
justifyContent: 'center',
...(p.fitted !== true &&
Copy link
Contributor

Choose a reason for hiding this comment

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

just to be sure - which case we are protecting against with explicit p.fitted !== true && .., instead of !p.fitted && ..?

Copy link
Member

Choose a reason for hiding this comment

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

const hasPadding = (fitted) => fitted !== true && fitted !== 'horizontally'
const hasPaddingModified = (fitted) => !fitted && fitted !== 'horizontally'
console.log(hasPadding(true), hasPadding('horizontally'), hasPadding('vertically'))
false false true
console.log(hasPaddingModified(true), hasPaddingModified('horizontally'), hasPaddingModified('vertically'))
false false false

I believe it's the reason

<p>
When <code>custom</code> kind is used it is the responsibility of the consumer to verify
accessibility and styling aspects of the component and handle them correctly. This kind of
items can't be actionable.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add:

This kind of items can't be actionable, but actionable components might be added to the content slot of the custom kind.

@layershifter layershifter added the ⚙️ enhancement New feature or request label Jul 2, 2019
@layershifter layershifter merged commit 9e11693 into master Jul 2, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/toolbar-custom-item branch July 2, 2019 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants