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

Feature/storage api #28

Merged
merged 18 commits into from
May 29, 2022
Merged

Feature/storage api #28

merged 18 commits into from
May 29, 2022

Conversation

Stalkakuma
Copy link
Contributor

@Stalkakuma Stalkakuma commented Apr 12, 2022

...
(stand in for commits I did not address, yet)
...

Packages added:

  1. jest-localstorage-mock -- added for Jest testing with Storage API.
  2. Updated Storybook packages from 6.4.21 to 6.4.22

Done:

  1. Two separate returns for the consent hook, returning either Cookies actions or if Storage API is passed as an argument -- storage actions.
  2. Three Separate storybook stories, to test out storage in a working environment.
  3. One extra Jest test using local storage.

TODO:

  1. Write the rest of the tests.
  2. Clean up code.

Base automatically changed from feature/storybook to main April 12, 2022 14:06
src/stories/CookieConsent.tsx Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #28 (e926b62) into main (d95974f) will increase coverage by 0.85%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   92.00%   92.85%   +0.85%     
==========================================
  Files           3        3              
  Lines          50       56       +6     
  Branches       17       22       +5     
==========================================
+ Hits           46       52       +6     
  Misses          3        3              
  Partials        1        1              
Impacted Files Coverage Δ
src/constants.ts 100.00% <100.00%> (ø)
src/useCookieConsent.ts 90.90% <100.00%> (+1.43%) ⬆️
src/utils.ts 100.00% <100.00%> (ø)

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 d95974f...e926b62. Read the comment docs.

@bring-shrubbery
Copy link
Owner

@Stalkakuma Another common thing people do is add a description in the PR, pointing to the ticket and explaining what changed. It's not required here, but would be nice as a practice. And you can do stuff like include "Closes !" text to automatically close the ticket after this PR is merged.

Copy link
Owner

@bring-shrubbery bring-shrubbery left a comment

Choose a reason for hiding this comment

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

Added some comments. Just a note on tests when you add them: the basic tests for the storage should essentially be the same as existing ones, but just use different storage; maybe it's even worth using the same tests, and iterate over them several times for different storage options.

src/stories/CookieConsent.tsx Outdated Show resolved Hide resolved
src/stories/CookieConsent.tsx Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/stories/CookieConsent.tsx Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/useCookieConsent.ts Show resolved Hide resolved
src/useCookieConsent.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
Copy link
Owner

@bring-shrubbery bring-shrubbery left a comment

Choose a reason for hiding this comment

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

Nice, I think we should merge Cypress tests first and then we can wrap up with this one!

@bring-shrubbery bring-shrubbery added the enhancement New feature or request label May 29, 2022
@bring-shrubbery bring-shrubbery merged commit b2bceb3 into main May 29, 2022
@bring-shrubbery bring-shrubbery deleted the feature/storage-api branch May 29, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants