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

feat: IPFS experiments UI #1048

Merged
merged 43 commits into from
Jun 12, 2019
Merged

feat: IPFS experiments UI #1048

merged 43 commits into from
Jun 12, 2019

Conversation

cwaring
Copy link
Member

@cwaring cwaring commented May 27, 2019

CleanShot 2019-05-27 at 16 17 21

This PR adds new functionality to enable/disable IPFS experiments from the settings UI.

Experiments are controlled from an EXPERIMENTS constant in src/bundles/experiments.js. In order to add a new experiment simply create a new key to assign a namespace and then add the appropriate values. The key will also be used to select the correct language strings from the i18n files. Defaults in English have been added.

npm: { // namespace
  action: async (enabled) => null, // default async callback that is executed on toggle with an `enabled` bool that can be used for switching methods if needed
  enabled: false, // default experiment state
  actionUrls: [ // meta urls that are rendered along the bottom of each experiment
    {
      url: 'https://github.com/ipfs-shipyard/npm-on-ipfs', // url for the a tag
      key: 'readMoreUrl' // translation key
    },
    {
      url: 'https://github.com/ipfs-shipyard/npm-on-ipfs/issues',
      key: 'issueUrl'
    },
    {
      url: 'https://github.com/ipfs-shipyard/npm-on-ipfs',
      key: 'feedbackUrl'
    }
  ],
  desktop: false // bool set to true if you only want the experiment to show in IPFS Desktop
}

Preflight checklist:

  • remove dummy data
  • set desktop to true for npm
    - [ ] add notifications for activation and error
  • add examples section description copy
  • analytics for enable/disable
  • tests?

fixes: #814
fixes: ipfs/ipfs-desktop#907

ref: ipfs/ipfs-desktop#911

@cwaring
Copy link
Member Author

cwaring commented May 27, 2019

Notes: there are a couple of dummy experiments included and desktop: false is setup on npm so that it is easier to review/test in your browser. I will amend these once we have the action functionality mapped in and we are ready to merge!

@cwaring
Copy link
Member Author

cwaring commented May 27, 2019

@hacdias I'm not sure how I should call out to enable npm-on-ipfs could you let me know when you have .a moment and I'll wire it up in the actions section. Thanks 🙌

src/bundles/experiments.js Outdated Show resolved Hide resolved
@hacdias
Copy link
Member

hacdias commented May 27, 2019

@hacdias I'm not sure how I should call out to enable npm-on-ipfs could you let me know when you have .a moment and I'll wire it up in the actions section. Thanks 🙌

Heeey! You can use doDesktopSettingsToggle('npmOnIpfs') to toggle the value. You can get the current value with selectDesktopSettings.npmOnIpfs. 😃 The feature is not yet merged on IPFS Desktop though.

@olizilla
Copy link
Member

@cwaring I'll review today, but just wanted to say that this is looking really nice!

src/bundles/experiments.js Outdated Show resolved Hide resolved
@cwaring
Copy link
Member Author

cwaring commented Jun 6, 2019

yeah it's a tricky one. I mean if the user reloaded the app or browser window then they could activate again but that may not be immediately obvious. perhaps something to revisit later on.

what might be helpful in determining this is if we wire in analytics to monitor enable/disable and time it took to activate an experiment.

@hacdias
Copy link
Member

hacdias commented Jun 6, 2019

I guess that would be a good idea, to add the analytics. Could you do that, please? If you don't have much time, I can take a look

@cwaring
Copy link
Member Author

cwaring commented Jun 6, 2019

I'm a bit pushed for time as I need to jump back onto camp but I can take a look early next week if you don't get chance before then :)

@hacdias
Copy link
Member

hacdias commented Jun 6, 2019

I'll do that today then hahaha. It'd be awesome to perhaps merge this today, or this week at most.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member

hacdias commented Jun 6, 2019

@cwaring done on cbbf1c7. Could you take a look please?

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
src/bundles/experiments.js Outdated Show resolved Hide resolved
src/bundles/experiments.js Outdated Show resolved Hide resolved
src/bundles/index.js Outdated Show resolved Hide resolved
@cwaring
Copy link
Member Author

cwaring commented Jun 6, 2019

@cwaring done on cbbf1c7. Could you take a look please?

does this track which experiment is being enabled/disabled and the time it took to perform the operation? (Sorry on my 📱 ATM so tricky to check)

@hacdias
Copy link
Member

hacdias commented Jun 6, 2019

@cwaring it should only track the time to perform the operation. I'm not sure how to include the experiment here though. I'm not really well aware of how the analytics work

@olizilla
Copy link
Member

olizilla commented Jun 6, 2019

In it's current form, the analytics bundle will just record that an experiment was toggled, but not which one, as it only records the action type. We are deliberately extra cautious in the analytics bundle to be very explicit about what is tracked, so we can give the user a clear answer on what is and isn't tracked.

Either we create an action per experiement, or we add custom logic into the analytics bundle for the experiments action to modify the action name to include the experiment name, here https://github.com/ipfs-shipyard/ipfs-webui/blob/babbb6d2d2e742df50fd169d5c5066cc9e2fc546/src/bundles/analytics.js#L109-L113

@cwaring
Copy link
Member Author

cwaring commented Jun 6, 2019

In it's current form, the analytics bundle will just record that an experiment was toggled, but not which one, as it only records the action type. We are deliberately extra cautious in the analytics bundle to be very explicit about what is tracked, so we can give the user a clear answer on what is and isn't tracked.

Either we create an action per experiement, or we add custom logic into the analytics bundle for the experiments action to modify the action name to include the experiment name, here

https://github.com/ipfs-shipyard/ipfs-webui/blob/babbb6d2d2e742df50fd169d5c5066cc9e2fc546/src/bundles/analytics.js#L109-L113

@olizilla great, we have a key in the action payload so it should be possible to pick that up here

@hacdias
Copy link
Member

hacdias commented Jun 6, 2019

@cwaring I passed the experiment name on the key. 6b0932e should do the trick!

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@daviddias
Copy link
Member

The most important question of all (:D), will my little IPFS Cube in my nav bar become the npm on IPFS cube?

image

image

It would feel it gets equipped with a super power!

@hacdias
Copy link
Member

hacdias commented Jun 10, 2019

@daviddias that's definitely an interesting idea. I just wonder how it'd scale - we only have three visible sides of the IPFS cube and we might introduce more experiments or even JS-IPFS support.

I've been wanting to make some changes to IPFS Desktop's icon so perhaps this is a good time to raise an issue there. Done: ipfs/ipfs-desktop#956.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@daviddias
Copy link
Member

I just wonder how it'd scale - we only have three visible sides of the IPFS cube and we might introduce more experiments or even JS-IPFS support.

I recommend to not overthink it :) You can potentially pick one side to be the "underlying implementation being used" and another for "the last experiment active" and once you have multiple useful experiments, then consider how to represent it.

@cwaring cwaring removed the WIP label Jun 10, 2019
@hacdias
Copy link
Member

hacdias commented Jun 11, 2019

@olizilla could you take one last look at this please?

Copy link
Contributor

@fsdiogo fsdiogo left a comment

Choose a reason for hiding this comment

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

LGTM, nice work 👍

@hacdias hacdias merged commit 7b15c76 into master Jun 12, 2019
@hacdias hacdias deleted the feat/experiments branch June 12, 2019 22:38
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.

Add opt-in npm-on-ipfs support Discover and try out experimental IPFS features
5 participants