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

feat(Alert): add fitted prop #1872

Merged
merged 7 commits into from
Sep 3, 2019
Merged

feat(Alert): add fitted prop #1872

merged 7 commits into from
Sep 3, 2019

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Sep 2, 2019

fitted prop

This PR adds a new prop to Alert component, it allows to only take up the width of its content.


Also adds an example into Usage with fixed container width that wraps Alert.

@codecov
Copy link

codecov bot commented Sep 2, 2019

Codecov Report

Merging #1872 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1872      +/-   ##
=========================================
- Coverage   69.72%   69.7%   -0.02%     
=========================================
  Files         886     886              
  Lines        7795    7797       +2     
  Branches     2256    2258       +2     
=========================================
  Hits         5435    5435              
- Misses       2350    2352       +2     
  Partials       10      10
Impacted Files Coverage Δ
...rc/themes/teams/components/Alert/alertVariables.ts 0% <ø> (ø) ⬆️
packages/react/src/components/Alert/Alert.tsx 80.95% <ø> (ø) ⬆️
...t/src/themes/teams/components/Alert/alertStyles.ts 6.06% <0%> (-0.4%) ⬇️

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 a35edad...3ff9f9f. Read the comment docs.

import * as React from 'react'

const AlertExampleWidth = () => {
const [width] = useRangeKnob({ name: 'width', initialValue: '500px' })
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to specify the bounds on knob instead? Let's create a ticket for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am working on PR that will introduce it

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it is #1876.

@@ -59,6 +59,9 @@ export interface AlertProps
/** Controls Alert's relation to neighboring items. */
attached?: boolean | 'top' | 'bottom'

/** An alert can only take up the width of its content. */
compact?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fitted? It feels awkward to have two different terms across the components for the same thing? If we decide that one is better then the other, we can rename all of them at once. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, fitted will work better there and will be consistent 👍

@vercel vercel bot temporarily deployed to staging September 2, 2019 16:10 Inactive
@layershifter layershifter changed the title feat(Alert): add compact prop feat(Alert): add fitted prop Sep 2, 2019
@@ -55,12 +56,13 @@ export default (siteVars: SiteVariablesPrepared): AlertVariables => {
color: siteVars.colors.grey[500],
fontWeight: siteVars.fontWeightRegular,
minHeight,
padding: `0 0 0 ${pxToRem(16)}`,
padding: `0 ${pxToRem(16)}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

@codepretty can you please check this one? Is it correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me

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.

4 participants