Skip to content

Add Snackbar #721

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

Merged
merged 22 commits into from
Jun 3, 2019
Merged

Add Snackbar #721

merged 22 commits into from
Jun 3, 2019

Conversation

nicknisi
Copy link
Member

@nicknisi nicknisi commented May 3, 2019

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Add a Snackbar widget

Properties:

  • type?: 'success' | 'error' - whether the Snackbar should display with its success or error state
  • open?: boolean; - Whether the Snackbar is open
  • message: string; - The title/text to display within the Snackbar
  • actionsRenderer?: () => DNode | DNode[]- Buttons to render in the actions portion of the Snackbar

Example use:

protected render() {
  return (
    <div id="example-autoclose">
        <h3>Auto close Snackbar</h3>
        <Button
            onClick={() => {
                this.open = true;
                setTimeout(() => {
                    this.open = false;
                }, 5000);
            }}
        >
            Show Snackbar
        </Button>
        <Snackbar
            type="success"
            open={this.open}
            actionsRenderer={() => <Button onClick={() => this._dismiss()}>Dismiss</Button>}
            message="Test Snackbar Error"
        />
    </div>
  );
}

Screenshot
image

Resolves #713

@nicknisi nicknisi marked this pull request as ready for review May 3, 2019 18:57
@nicknisi nicknisi requested a review from tomdye May 3, 2019 18:57
@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #721 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
+ Coverage   98.41%   98.42%   +0.01%     
==========================================
  Files          44       46       +2     
  Lines        3273     3294      +21     
  Branches      970      973       +3     
==========================================
+ Hits         3221     3242      +21     
  Misses         52       52
Impacted Files Coverage Δ
src/snackbar/index.tsx 100% <100%> (ø)
src/snackbar/nls/Snackbar.ts 100% <100%> (ø)

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 0ef5afa...4a90fc0. Read the comment docs.

@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #721 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
+ Coverage    98.4%   98.42%   +0.02%     
==========================================
  Files          44       47       +3     
  Lines        3257     3300      +43     
  Branches      968      976       +8     
==========================================
+ Hits         3205     3248      +43     
  Misses         52       52
Impacted Files Coverage Δ
src/snackbar/index.tsx 100% <100%> (ø)
src/card/index.tsx 100% <0%> (ø)
src/outlined-button/index.tsx 100% <0%> (ø)

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 658e4b3...4c8181e. Read the comment docs.

@nicknisi nicknisi requested a review from matt-gadd May 6, 2019 18:36
@@ -0,0 +1,54 @@
@import '../common/styles/variables.css';

.root {
Copy link
Member

Choose a reason for hiding this comment

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

not sure if this should all be here or in the dojo theme?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tomdye Good question. I added the minimum structure and positioning to this CSS, just so that it uses flex box and displays at the bottom of the window. Should this not be here?

export class Snackbar extends WidgetBase<SnackbarProperties> {
protected render(): DNode {
const { type, open, message, actionsRenderer } = this.properties;
const classes = [css.root];
Copy link
Member

Choose a reason for hiding this comment

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

Need to make Widget themable

nicknisi added 14 commits May 18, 2019 09:19
* make `open` property not optional
* remove unnecessary manual typing
* simplify adding classes, remove nested ternary
- Allow actions to be passed, which are buttons that will be rendered on
the Snackbar
- Replace `success` with `type?: 'success' | 'error'`
- type return value of render method
- remove nls for Snackbar and use of I18nMixin
export class Snackbar extends ThemedMixin(WidgetBase)<SnackbarProperties> {
protected render(): DNode {
const { type, open, leading, stacked, message, actionsRenderer } = this.properties;
const classes = [css.root];
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you didn't use ternary for adding conditional classes to the root here like we do elsewhere.

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 changed this to be consistent with what we've done elsewhere. I originally did it this way because I'm not a fan of testing the placement of null.

const successTemplate = template.setProperty('@root', 'classes', [
	css.root,
	css.open,
	null,
	null,
	css.stacked
]);

<div key="label" classes={css.label} role="status" aria-live="polite">
{message}
</div>
<div key="actions" classes={css.actions}>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure the actions section should be rendered at all if there is no actionsRenderer property passed

const { type, open, leading, stacked, message, actionsRenderer } = this.properties;
const classes = [css.root];

if (open) {
Copy link
Member

Choose a reason for hiding this comment

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

Please see previous comment re. ternary for these classes as we have done elsewhere.

@nicknisi nicknisi merged commit 937ff2b into dojo:master Jun 3, 2019
@nicknisi nicknisi deleted the snackbar branch June 3, 2019 16:26
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.

Snackbar
4 participants