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.

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

OEL-779: Review patterns: Accordion, Alert, Badge, Banner, Blockquote and Breadcrumbs #150

Merged
merged 27 commits into from
Jan 21, 2022

Conversation

escuriola
Copy link
Contributor

Jira issue(s):

Added quote paragraph testing.

@escuriola escuriola force-pushed the OEL-779-part-one branch 3 times, most recently from 6a8ad61 to 4407525 Compare January 7, 2022 15:43
@escuriola escuriola changed the title OEL-779: Review patterns: Accordion, Alert, Badge, Banner and Blockquote OEL-779: Review patterns: Accordion, Alert, Badge, Banner, Blockquote and Breadcrumbs Jan 7, 2022
type: boolean
label: "Flush"
description: "set Flush to remove the default background-color, some borders, and some rounded corners to render accordions edge-to-edge with their parent container."
label: 'Flush'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change flush to "bare layout"

light: 'light'
dark: 'dark'
link: 'link'
preview: 'primary'
Copy link
Contributor

Choose a reason for hiding this comment

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

Default value is not necessary to be added here.

preview: True
label: 'Dismissible'
description: 'Add close button to dismiss the alert inline. Enabled by default.'
preview: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Dismissible default value is not necessary to report it here.

preview: True
label: 'Animated dismiss'
description: 'Add fade animation to the dismissible alert. Enabled by default.'
preview: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, is not necessary, eliminate it at markup rendering test if present.

light: 'light'
dark: 'dark'
link: 'link'
preview: 'primary'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not necessary to put the default. Can be deleted at markup test if present.

description: "Enable the shadow effect to the image."
type: boolean
label: 'Shade'
description: 'Enable the shadow effect to the image. False by default.'
preview: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary. Can be deleted. Check if present at markup test.

description: "Display banner as hero banner"
type: boolean
label: 'Hero'
description: 'Display as hero banner. False by default.'
preview: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary. Can be deleted. Check if present at markup test.

description: "Possibility of having a full-width banner within a bootstrap container"
type: boolean
label: 'Full width'
description: 'Possibility of having a full-width banner within a bootstrap container. False by default.'
preview: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary. Can be deleted. Check if present at markup test.

icon:
name: "chevron-right"
name: 'chevron-right'
size: s
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary, 's' is the default size. Can be deleted. Leave the assertion for icon having size 's'.

left: 'left'
center: 'center'
end: 'end'
preview: 'left'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary. Eliminate the setting of this at markup tests.

label: Alert
description: Provide contextual feedback messages for typical user actions with the handful of available and flexible alert messages. https://v5.getbootstrap.com/docs/5.0/components/alerts/
label: 'Alert'
description: 'Provide contextual feedback messages for typical user actions with the handful of available and flexible alert messages. https://v5.getbootstrap.com/docs/5.0/components/alerts/'
settings:
style:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we don't use variants for this? I think it fits more the scope of variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Variant is a reserved word and doesn't work if there are no variants (patterns with different behaviours) in the pattern.

preview: "/assets/icons/bootstrap-icons.svg"
label: 'Message'
description: 'The alert message.'
preview: 'A simple alert—check it out!'
icon_name:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we simplify this and use "icon:" ?

type: text
label: 'Description'
description: 'Sub-heading of the banner.'
preview: 'Innovation, economy, environment and geopolitics'
link:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe rename this, since it's not just a link?

@@ -1,11 +1,11 @@
accordion:
accordion_with_flush:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe accordion_with_bare_layout?

brummbar
brummbar previously approved these changes Jan 19, 2022
drishu
drishu previously approved these changes Jan 20, 2022
@brummbar brummbar merged commit 7ccae45 into 1.x Jan 21, 2022
@brummbar brummbar deleted the OEL-779-part-one branch January 21, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants