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

docs(modal): introduce title-only modal examples #4703

Merged
merged 6 commits into from
Nov 21, 2019
Merged

docs(modal): introduce title-only modal examples #4703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
54bc6ef
docs(modal): introduce title-only modal examples
asudoh Nov 18, 2019
1f5f2ad
Merge branch 'master' into modal-titleonly
abbeyhrt Nov 18, 2019
8fdc5e9
fix(modal): add title only modal to have footer
asudoh Nov 19, 2019
110110c
docs(Modal): show danger knob only in transactional variant
asudoh Nov 20, 2019
6f2689d
Merge remote-tracking branch 'upstream/master' into modal-titleonly
asudoh Nov 20, 2019
9e8366f
Merge branch 'master' into modal-titleonly
asudoh Nov 20, 2019
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
57 changes: 57 additions & 0 deletions packages/components/src/components/modal/modal.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const { prefix } = require('../../globals/js/settings');
module.exports = {
context: {
prefix,
hasBody: true,
},
variants: [
{
Expand Down Expand Up @@ -120,6 +121,62 @@ module.exports = {
size: 'lg',
},
},
{
name: 'titleonly-xs',
label: 'Title Only Modal (XS)',
context: {
idSuffix: Math.random()
.toString(36)
.substr(2),
hasBody: false,
hasFooter: true,
classPrimaryButton: `${prefix}--btn--primary`,
classCloseButton: `${prefix}--btn--secondary`,
size: 'xs',
},
},
{
name: 'titleonly-sm',
label: 'Title Only Modal (Small)',
context: {
idSuffix: Math.random()
.toString(36)
.substr(2),
hasBody: false,
hasFooter: true,
classPrimaryButton: `${prefix}--btn--primary`,
classCloseButton: `${prefix}--btn--secondary`,
size: 'sm',
},
},
{
name: 'titleonly-nofooter-xs',
label: 'Title Only Passive Modal (XS)',
context: {
idSuffix: Math.random()
.toString(36)
.substr(2),
hasBody: false,
hasFooter: false,
classPrimaryButton: `${prefix}--btn--primary`,
classCloseButton: `${prefix}--btn--secondary`,
size: 'xs',
},
},
{
name: 'titleonly-nofooter-sm',
label: 'Title Only Passive Modal (Small)',
context: {
idSuffix: Math.random()
.toString(36)
.substr(2),
hasBody: false,
hasFooter: false,
classPrimaryButton: `${prefix}--btn--primary`,
classCloseButton: `${prefix}--btn--secondary`,
size: 'sm',
},
},
{
name: 'danger',
label: 'Danger Modal',
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/components/modal/modal.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<div class="{{@root.prefix}}--modal-container{{#if size}} {{@root.prefix}}--modal-container--{{size}}{{/if}}">
<div class="{{@root.prefix}}--modal-header">
<p class="{{@root.prefix}}--modal-header__label {{@root.prefix}}--type-delta" id="modal-{{idSuffix}}-label">Optional label</p>
<p class="{{@root.prefix}}--modal-header__heading {{@root.prefix}}--type-beta" id="modal-{{idSuffix}}-heading">Modal heading</p>
<p class="{{@root.prefix}}--modal-header__heading {{@root.prefix}}--type-beta" id="modal-{{idSuffix}}-heading">{{#if hasBody}}Modal heading{{else}}Passive modal title as the message. Should be direct and 3 lines or less.{{/if}}</p>
<button class="{{@root.prefix}}--modal-close" type="button" data-modal-close aria-label="close modal" {{#unless hasFooter}} data-modal-primary-focus{{/unless}}>
{{ carbon-icon 'Close16' class=(add @root.prefix '--modal-close__icon') }}
</button>
Expand All @@ -28,7 +28,7 @@
<input id="text-input-{{idSuffix}}" type="text" class="{{@root.prefix}}--text-input" placeholder="Optional placeholder text"
data-modal-primary-focus>
</div>
{{else}}
{{else if hasBody}}
<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Aenean id accumsan augue. Phasellus consequat augue
vitae
tellus tincidunt posuere. Curabitur justo urna, consectetur vel elit iaculis, ultrices condimentum risus. Nulla
Expand Down
38 changes: 33 additions & 5 deletions packages/react/src/components/ComposedModal/ComposedModal-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,26 @@ const sizes = {
};

const props = {
composedModal: (includeOpen = true) => ({
open: includeOpen ? boolean('Open (open in <ComposedModal>)', true) : null,
composedModal: ({ titleOnly } = {}) => ({
open: boolean('Open (open in <ComposedModal>)', true),
onKeyDown: action('onKeyDown'),
danger: boolean('Danger mode (danger)', false),
selectorPrimaryFocus: text(
'Primary focus element selector (selectorPrimaryFocus)',
'[data-modal-primary-focus]'
),
size: select('Size (size)', sizes),
size: select('Size (size)', sizes, titleOnly ? 'sm' : ''),
}),
modalHeader: () => ({
modalHeader: ({ titleOnly } = {}) => ({
label: text('Optional Label (label in <ModalHeader>)', 'Optional Label'),
title: text('Optional title (title in <ModalHeader>)', 'Example'),
title: text(
'Optional title (title in <ModalHeader>)',
titleOnly
? `
Passive modal title as the message. Should be direct and 3 lines or less.
`.trim()
: 'Example'
),
iconDescription: text(
'Close icon description (iconDescription in <ModalHeader>)',
'Close'
Expand Down Expand Up @@ -191,6 +198,27 @@ storiesOf('ComposedModal', module)
},
}
)
.add(
'Title only',
() => {
const { size, ...rest } = props.composedModal({ titleOnly: true });
return (
<ComposedModal {...rest} size={size || undefined}>
<ModalHeader {...props.modalHeader({ titleOnly: true })} />
<ModalBody />
<ModalFooter {...props.modalFooter()} />
</ComposedModal>
);
},
{
info: {
text: `
In "small" and "xs" modals size, the title is allowed to span multiple lines and be used for the main message.
It should be less than 3 lines of text. If more room is required then use the standard body copy format.
`,
},
}
)
.add(
'Example usage with trigger button',
() => {
Expand Down
56 changes: 56 additions & 0 deletions packages/react/src/components/Modal/Modal-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,43 @@ const props = () => ({
onSecondarySubmit: action('onSecondarySubmit'),
});

const titleOnlyProps = () => {
const passiveModal = boolean('Without footer (passiveModal)', false);
return {
className: 'some-class',
open: boolean('Open (open)', true),
passiveModal,
danger: !passiveModal && boolean('Danger mode (danger)', false),
modalHeading: text(
'Modal heading (modalHeading)',
`
Passive modal title as the message. Should be direct and 3 lines or less.
`.trim()
),
modalAriaLabel: text(
'ARIA label, used only if modalLabel not provided (modalAriaLabel)',
'A label to be read by screen readers on the modal root node'
),
primaryButtonText: text(
'Primary button text (primaryButtonText)',
'Primary Button'
),
secondaryButtonText: text(
'Secondary button text (secondaryButtonText)',
'Secondary Button'
),
size: select('Size (size)', sizes, 'sm'),
iconDescription: text(
'Close icon description (iconDescription)',
'Close the modal'
),
onBlur: action('onBlur'),
onClick: action('onClick'),
onFocus: action('onFocus'),
onRequestClose: action('onRequestClose'),
};
};

storiesOf('Modal', module)
.addDecorator(withKnobs)
.add(
Expand Down Expand Up @@ -153,6 +190,25 @@ storiesOf('Modal', module)
},
}
)
.add(
'Title only',
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we could have this new example be without knobs? As we're adding stories, might make more sense to drop knob usage as they can make it harder to see how to use the component from the story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The knobs are still needed for heading content, etc. I think the discussion was around a story variant like this title only example, which was introduced as a story variant instead of a knob.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure I understand. Just was speaking to the Title only example being hard to know how to use because of the knobs. In other words, this snippet doesn't really help an external user understand how to use the component easily as it has the knob abstraction applied to it

image

This is something that's been coming up in our dev channels where we have been talking about not using knobs because they can make it harder to use Storybook as a reference for how to use a component.

Copy link
Contributor Author

@asudoh asudoh Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see your point about the snippet readability. I'd like to keep the ability to demonstrate different options here, esp. to keep the consistency of other modal demos.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't to suggest that we have to remove how we demonstrate these options, just was speaking to how we're choosing to demonstrate these options through knobs and how that makes things hard for existing content. Would be great if new content didn't fall into the same problem 👍

Copy link
Contributor Author

@asudoh asudoh Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want to include that to your v11 proposal list? I'd like to keep the knobs usage consistent across other modal stories for this PR, but we can always revisit the pattern in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely understand 👍 Since it's related to docs, doubt we need to wait for v11 but can be an effort over time to update.

() => {
const { size, ...rest } = titleOnlyProps();
return (
<>
<Modal {...rest} size={size || undefined}></Modal>
</>
);
},
{
info: {
text: `
In "small" and "xs" modals size, the title is allowed to span multiple lines and be used for the main message.
It should be less than 3 lines of text. If more room is required then use the standard body copy format.
`,
},
}
)
.add(
'Trap Focus',
() => {
Expand Down