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.

Sign up for GitHub

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

Jump to bottom

active PropTypes on MobileLayout #6241

Merged
merged 4 commits into from
Apr 26, 2019
Merged

active PropTypes on MobileLayout #6241

merged 4 commits into from
Apr 26, 2019

Conversation

ShimiSun
Copy link
Contributor

@ShimiSun ShimiSun commented Mar 23, 2019 •
edited by shilman
Loading

Issue: #5910

Not sure if this is the right approach, but active on Panels is being fetched from Mobile, and Mobile treats active as bool (L-132), hence I assume the active PropTypes is missing the support of boolean.
This should solve the following warning message :

vendors~main.fe9333fdaa91c3bc407a.bundle.js:8776 Warning: Failed prop type: 
Invalid prop `active` of type `boolean` supplied to `<<anonymous>>`, 
expected `number`.
    in Unknown (created by MobileLayout)
    in MobileLayout (created by ResizeDetector)
    in ResizeDetector
    in Unknown
    in Unknown (created by Manager)
    in ThemeProvider (created by Manager)
    in Manager (created by Context.Consumer)
    in Location (created by QueryLocation)
    in QueryLocation (created by Root)
    in LocationProvider (created by Root)
    in HelmetProvider (created by Root)
    in Root

Issue:

What I did

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@ShimiSun
active PropTypes on MobileLayout
eed7619 
Not sure if that is the right approach, but `active` on Panels is being fetched from Mobile, and Mobile treats `active` as bool (L-132), hence I assume the `active` PropTypes is missing the support of boolean.
This should solve the following warning message :
```
vendors~main.fe9333fdaa91c3bc407a.bundle.js:8776 Warning: Failed prop type: Invalid prop `active` of type `boolean` supplied to `<<anonymous>>`, expected `number`.
    in Unknown (created by MobileLayout)
    in MobileLayout (created by ResizeDetector)
    in ResizeDetector
    in Unknown
    in Unknown (created by Manager)
    in ThemeProvider (created by Manager)
    in Manager (created by Context.Consumer)
    in Location (created by QueryLocation)
    in QueryLocation (created by Root)
    in LocationProvider (created by Root)
    in HelmetProvider (created by Root)
    in Root
```
@shilman shilman added bug ui patch:yes Bugfix & documentation PR that need to be picked to main branch labels Mar 23, 2019
@shilman shilman added this to the 5.0.x milestone Mar 23, 2019
shilman
shilman approved these changes Mar 23, 2019
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thank you @ShimiSun! @ndelangen mind taking a look at this?

@Armanio
Copy link
Member

Armanio commented Mar 26, 2019

Hmm, I think we need to fix initial state in Mobile component. On component start state.active has boolean value.

@ndelangen
Copy link
Member

I think the problem isn't that the PropTypes are incorrect, I think we're passing the wrong value to the component.

@BPScott BPScott mentioned this pull request Apr 4, 2019
Merged
@Armanio
Copy link
Member

Armanio commented Apr 13, 2019

@ShimiSun could you fix initial state on Mobile component? here
If not let me know - I fix it self.
Thank you for effort!

@iamandrewluca
Copy link
Contributor

Use this GitHub Tip for better linking between issue and PR :)
https://help.github.com/en/articles/closing-issues-using-keywords

@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #6241 into next will decrease coverage by 2.89%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             next    #6241     +/-   ##
=========================================
- Coverage   41.01%   38.12%   -2.9%     
=========================================
  Files         613      645     +32     
  Lines        8460     9647   +1187     
  Branches      377      355     -22     
=========================================
+ Hits         3470     3678    +208     
- Misses       4937     5914    +977     
- Partials       53       55      +2
Impacted Files Coverage Δ
lib/ui/src/components/layout/mobile.js 77.14% <ø> (ø) ⬆️
addons/actions/src/preview/action.ts 90.9% <0%> (-9.1%) ⬇️
lib/core/src/client/preview/start.js 72.94% <0%> (-0.93%) ⬇️
addons/knobs/src/components/Panel.js 79.72% <0%> (-0.28%) ⬇️
lib/ui/src/components/sidebar/treeview/utils.js 83.87% <0%> (-0.18%) ⬇️
addons/ondevice-knobs/src/types/Date.js 0% <0%> (ø) ⬆️
app/polymer/src/client/index.js 0% <0%> (ø) ⬆️
app/html/standalone.js 0% <0%> (ø) ⬆️
lib/core/src/server/config.js 0% <0%> (ø) ⬆️
lib/core/src/server/cli/prod.js 0% <0%> (ø) ⬆️
... and 339 more

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 2564834...06b8547. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 17, 2019 •
edited
Loading

Codecov Report

Merging #6241 into next will decrease coverage by 2.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6241      +/-   ##
==========================================
- Coverage   40.44%   38.12%   -2.33%     
==========================================
  Files         637      645       +8     
  Lines        8759     9647     +888     
  Branches      639      355     -284     
==========================================
+ Hits         3543     3678     +135     
- Misses       5118     5914     +796     
+ Partials       98       55      -43
Impacted Files Coverage Δ
lib/ui/src/components/layout/mobile.js 77.14% <ø> (ø) ⬆️
addons/actions/src/preview/action.ts 90.9% <0%> (-9.1%) ⬇️
app/angular/src/server/angular-cli_utils.js 43.75% <0%> (-3.13%) ⬇️
addons/knobs/src/components/Panel.js 79.72% <0%> (-1.98%) ⬇️
lib/client-api/src/story_store.js 74.57% <0%> (-1.74%) ⬇️
lib/core/src/client/preview/start.js 72.94% <0%> (-0.93%) ⬇️
lib/ui/src/components/sidebar/treeview/utils.js 83.87% <0%> (-0.18%) ⬇️
addons/ondevice-knobs/src/types/Date.js 0% <0%> (ø) ⬆️
app/polymer/src/client/index.js 0% <0%> (ø) ⬆️
app/html/standalone.js 0% <0%> (ø) ⬆️
... and 520 more

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 a52efc7...3063ca2. Read the comment docs.

@ShimiSun
Copy link
Contributor Author

@Armanio done, let me know if that works.

@ndelangen ndelangen self-assigned this Apr 26, 2019
@ndelangen
Copy link
Member

I'll fix the tests and merge

@vercel
Copy link

vercel bot commented Apr 26, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-shimisun-patch-3.storybook.now.sh

@ndelangen ndelangen merged commit a29640e into storybookjs:next Apr 26, 2019
@shilman shilman mentioned this pull request Apr 26, 2019
Closed
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shilman shilman shilman approved these changes

@igor-dv igor-dv Awaiting requested review from igor-dv

@ndelangen ndelangen Awaiting requested review from ndelangen

@tmeasday tmeasday Awaiting requested review from tmeasday

Assignees

@ndelangen ndelangen

Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch ui
Projects
None yet
Milestone
5.0.x
Development

Successfully merging this pull request may close these issues.
5 participants
@ShimiSun @Armanio @ndelangen @iamandrewluca @shilman