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

Ondevice actions #6594

Merged
merged 11 commits into from
May 2, 2019
Merged

Ondevice actions #6594

merged 11 commits into from
May 2, 2019

Conversation

ForbesLindesay
Copy link
Contributor

Issue:

What I did

How to test

  • Is this testable with Jest or Chromatic screenshots? - maybe
  • Does this need a new example in the kitchen sink apps? - I've added it to crna
  • Does this need an update to the documentation? - I've updated ADDONS_SUPPORT

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

It looks like this:

Screenshot 2019-04-23 at 14 10 59
Screenshot 2019-04-23 at 14 11 29

Forbes Lindesay added 4 commits April 18, 2019 16:07
This adds an ondevice actions addon that shows a logger for the actions on device.
The "API" passed to react native addons is not the same kind of API passed to web addons, so the previous code failed.
@vercel
Copy link

vercel bot commented Apr 23, 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-forbeslindesay-ondevice-actions.storybook.now.sh

@shilman
Copy link
Member

shilman commented Apr 23, 2019

@ForbesLindesay awesome awesome awesome 💯 this is really exciting!

@shilman shilman added this to the 5.1.0 milestone Apr 23, 2019
"extends": "../../tsconfig.json",
"compilerOptions": {
"rootDir": "./src",
"types": ["webpack-env"]
Copy link
Member

Choose a reason for hiding this comment

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

this may not be needed

},
"license": "MIT",
"main": "dist/index.js",
"jsnext:main": "src/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

since this package is typescript only you shouldn't need this.

"core-js": "^2.5.7",
"fast-deep-equal": "^2.0.1",
"global": "^4.3.2",
"lodash": "^4.17.11",
Copy link
Member

Choose a reason for hiding this comment

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

are all these dependencies used by your addon?

"publishConfig": {
"access": "public"
},
"gitHead": "fbd7ee4c80df437fed4bdc6e11140733fd450080"
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be required

@benoitdion
Copy link
Member

hey @ForbesLindesay, this is awesome! A few minor comments but looks really good overall.

Do you mind adding a story in the crna-kitchen-sink app to showcase usages of this new addon?

@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #6594 into next will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6594      +/-   ##
==========================================
- Coverage   40.65%   40.42%   -0.24%     
==========================================
  Files         633      621      -12     
  Lines        8672     8603      -69     
  Branches      620      618       -2     
==========================================
- Hits         3526     3478      -48     
+ Misses       5057     5033      -24     
- Partials       89       92       +3
Impacted Files Coverage Δ
addons/actions/src/index.ts 0% <ø> (ø) ⬆️
...vice-actions/src/components/ActionLogger/index.tsx 0% <0%> (ø)
...ce-actions/src/components/ActionLogger/Inspect.tsx 0% <0%> (ø)
addons/ondevice-actions/src/index.tsx 0% <0%> (ø)
addons/ondevice-actions/register.js 0% <0%> (ø)
...vice-actions/src/containers/ActionLogger/index.tsx 0% <0%> (ø)
addons/backgrounds/src/constants.ts 0% <0%> (ø) ⬆️
addons/info/src/components/types/PrettyPropType.js 41.66% <0%> (ø) ⬆️
addons/info/src/components/types/OneOfType.js 28.57% <0%> (ø) ⬆️
lib/core/src/server/build-dev.js 0% <0%> (ø) ⬆️
... and 32 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 3941007...2ec925c. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #6594 into next will decrease coverage by 0.33%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6594      +/-   ##
==========================================
- Coverage   40.26%   39.92%   -0.34%     
==========================================
  Files         640      645       +5     
  Lines        8810     8884      +74     
  Branches      643      666      +23     
==========================================
  Hits         3547     3547              
- Misses       5164     5238      +74     
  Partials       99       99
Impacted Files Coverage Δ
addons/actions/src/index.ts 0% <ø> (ø) ⬆️
addons/ondevice-actions/register.js 0% <0%> (ø)
...vice-actions/src/containers/ActionLogger/index.tsx 0% <0%> (ø)
...ce-actions/src/components/ActionLogger/Inspect.tsx 0% <0%> (ø)
...vice-actions/src/components/ActionLogger/index.tsx 0% <0%> (ø)
addons/ondevice-actions/src/index.tsx 0% <0%> (ø)
... and 1 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 dfc0772...3990ba7. Read the comment docs.

@ForbesLindesay
Copy link
Contributor Author

@benoitdion Thanks for the feedback. I've hopefully resolved all of it now. I had committed but forgotten to push the update to crna-kitchen-sink. It doesn't actually need a new example as the existing examples already have actions, so simply adding the new ondevice-actions addon does the trick.

@benoitdion
Copy link
Member

benoitdion commented Apr 24, 2019

Thanks for taking the time to address the feedback @ForbesLindesay! It looks like your build is failing, you may need to run yarn.

I took it out for a spin and it's working great but the ui looks a bit different for me:
Screenshot_1556106111

do you know what might be going on with the expand/collapse buttons? I wonder if we could decrease the margin added to indent to decrease horizontal scrolling.

After expanding a couple actions the clear button disappears. It may need to be part of the ScrollView or sticky at the bottom.

What do you think?

@ndelangen
Copy link
Member

WOW awesome contribution @ForbesLindesay

@benoitdion looks like RN is really getting the attention it deserves, are you able to handle this as well?

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

I'll fix the yarn lock situation

# Conflicts:
#	examples-native/crna-kitchen-sink/package.json
@ForbesLindesay
Copy link
Contributor Author

After expanding a couple actions the clear button disappears.

It's in a scroll view, so this works fine for me. I only tested on iOS though

do you know what might be going on with the expand/collapse buttons?

I only tested on iOS, I think probably the Buttons are styled differently on the different platforms, probably we should be using something like TouchableHighlight instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants