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

[Incomplete] Feature: Snackbar implementation #620

Merged
merged 1 commit into from Jan 29, 2019

Conversation

gugu
Copy link
Contributor

@gugu gugu commented Jan 16, 2019

The initial version of Snackbar. If code will be in general OK, I'll add tests as well

If you want to try: npm install @shortcm/snackbar

@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #620 into rc0.10.0 will increase coverage by 0.04%.
The diff coverage is 96.82%.

Impacted file tree graph

@@             Coverage Diff              @@
##           rc0.10.0     #620      +/-   ##
============================================
+ Coverage     94.83%   94.87%   +0.04%     
============================================
  Files            67       68       +1     
  Lines          2768     2831      +63     
  Branches        414      426      +12     
============================================
+ Hits           2625     2686      +61     
  Misses           50       50              
- Partials         93       95       +2
Impacted Files Coverage Δ
packages/snackbar/index.tsx 96.82% <96.82%> (ø)

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 ae38323...9d44be8. Read the comment docs.

Copy link
Contributor

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

Initial thoughts: code looks good, but things are missing.

foundation.close()
handleKeyDown()
getTimeoutMs()
getCloseOnEscape()
isOpen() // can be used through props.

packages/snackbar/index.tsx Outdated Show resolved Hide resolved
packages/snackbar/package.json Outdated Show resolved Hide resolved
packages/snackbar/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

Good initial set of tests, but need more coverage. We're aiming for 95% lines/functions/statements/branches. I know we are lacking in some areas, which we need to address. But we don't want to fall further behind. Added some comments.

packages/snackbar/README.md Outdated Show resolved Hide resolved
packages/snackbar/README.md Outdated Show resolved Hide resolved
packages/snackbar/README.md Outdated Show resolved Hide resolved
packages/snackbar/README.md Outdated Show resolved Hide resolved
packages/snackbar/index.tsx Outdated Show resolved Hide resolved
packages/snackbar/package.json Outdated Show resolved Hide resolved
packages/snackbar/types.tsx Show resolved Hide resolved
scripts/package-json-reader.js Outdated Show resolved Hide resolved
test/screenshot/snackbar/index.scss Outdated Show resolved Hide resolved
test/unit/snackbar/index.test.tsx Show resolved Hide resolved
@moog16
Copy link
Contributor

moog16 commented Jan 23, 2019

fixes #447

packages/snackbar/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

I think the last piece is adding adapter tests to achieve 95% test coverage.

@moog16
Copy link
Contributor

moog16 commented Jan 24, 2019

@gugu I see you did more tests! If you run npm run test:unit, this will create a coverage directory. Can you make sure that branches, statements, lines, and functions is +95%. If you can't do it let me know. From the looks of it you're around:

86.96% Statements 60/69 75% Branches 18/24 65% Functions 13/20 85.48% Lines 53/62

You can also see line/by/line coverage. If you have any trouble, let me know and we can work through it.

@moog16 moog16 changed the base branch from rc0.9.0 to rc0.10.0 January 24, 2019 17:13
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@gugu
Copy link
Contributor Author

gugu commented Jan 24, 2019

Done. It now says about CLA signing, probably after merge

@moog16
Copy link
Contributor

moog16 commented Jan 24, 2019

looks like you have some conflicting files. Also I only see a commit that updated karma. Did you forget some files in your last commit?

@gugu gugu force-pushed the react-snackbar branch 2 times, most recently from 3358695 to 7365f13 Compare January 25, 2019 11:18
@googlebot
Copy link

CLAs look good, thanks!

@gugu gugu force-pushed the react-snackbar branch 2 times, most recently from fc6a2c0 to 002ddca Compare January 25, 2019 11:22
@gugu
Copy link
Contributor Author

gugu commented Jan 25, 2019

The problem was when the base branch was changed. I removed extra commits and squashed my changes to one commit (to not to pollute module history). Also I added karma config instead of tests. Now test coverage is 100%

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

I realized that you also need to add snackbar to the screenshot-test-urls.tsx file, and water with rc0.10.0.

@gugu
Copy link
Contributor Author

gugu commented Jan 25, 2019

Done with docs and screenshot-test-urls. What does "water with rc0.10.0." mean? I did rebase from rc0.10.0, but I'm not sure it is what did you mean

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

water === merge...the more and more I say that, the more I think it is not a real term. Rebase is fine :)

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

@gugu created a screenshot. please update snackbar golden.json file:
fb7f6f61f37095d7e7c92c9305eb797145d8e72c0c8dbc4e531f9aca91b0fd28

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

i made the gc bucket public (I think)...try running npm run lint.

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

tests for #637

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

@gugu did you sign PR...i remember you doing so.

@gugu
Copy link
Contributor Author

gugu commented Jan 25, 2019

I signed it

@gugu
Copy link
Contributor Author

gugu commented Jan 25, 2019

Looks like this message I need to write to sign

@moog16
Copy link
Contributor

moog16 commented Jan 25, 2019

Thanks :)

Fix default classes

WIP correct implementation of announce method

Change modile name, onAnnounce event, handling keydown

Add missing methods, unit tests

Minimal screenshot tests

Leading/stacked tests

React-snackbar example

ESLint

Improve quality of react-snackbar (quick fixes)

Add comments for snackbar test css hack, standard callback names in props

Add TODO for updating types from mdc-snackbar when possible

Added docs for announce, version 0.0.0, typescript lint fixes

Adapter tests, moved mdc-snackbar to classes attribute

Achieve 100% test coverage

Revert karma changes

Getting snackbar parameters docs
@gugu
Copy link
Contributor Author

gugu commented Jan 29, 2019

rebased to the last rc0.10.0 commit

@moog16 moog16 merged commit a2a4473 into material-components:rc0.10.0 Jan 29, 2019
moog16 pushed a commit that referenced this pull request Jan 30, 2019
moog16 pushed a commit that referenced this pull request Feb 19, 2019
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

5 participants