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

Convert to React views & Redux state management from Ampersand views & models #1307

Closed
lmorchard opened this Issue Aug 29, 2016 · 20 comments

Comments

Projects
None yet
5 participants
@lmorchard
Member

lmorchard commented Aug 29, 2016

This work in progress has been merged into the development-static branch and is auto-deploying to https://testpilot-static.dev.mozaws.net

if you set testpilot.env to static-dev in about:config, this environment should fully work. Anything not listed as a known issue in the TODOs below is something new and needs to be fixed - so feel free to edit this & add to the list!

To run the development-static branch locally, just check it out and then run this:

cd addon
npm install && npm run package
cd ..
npm install
while [ 1 ]; do NODE_ENV=development gulp; sleep 1; done

Docker is no longer needed. Just build the add-on and start up gulp. The while loop is to re-start gulp when it exits, which it will do if it sees edits to gulpfile.js or build tasks.

Note: If you get an "Unexpected token" error when trying to run gulp, make sure you're running at least Node v6.2.0.

Remaining TODOs:

  • Auto-deployment does not invalidate CloudFront distribution, so things stay stale after deployment
  • Links in Settings menu do the wrong things, retire flow is broken.
  • Port over the new mozAddonManager code for add-on installation
  • #1349 [ ] Try building with React in production mode, to be sure everything still works
  • #1350 [ ] Update docs to describe new static generation & React architecture. No more Docker.
  • #1351 [ ] Frontend tests - port over existing? start fresh with Mocha + Enzyme?

@lmorchard lmorchard added this to the TXP-32 Frontend Migration milestone Aug 29, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 2, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 3, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 4, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 4, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 4, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 5, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 5, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit to lmorchard/testpilot that referenced this issue Sep 6, 2016

lmorchard added a commit that referenced this issue Sep 12, 2016

lmorchard added a commit that referenced this issue Sep 12, 2016

lmorchard added a commit that referenced this issue Sep 13, 2016

lmorchard added a commit that referenced this issue Sep 13, 2016

lmorchard added a commit that referenced this issue Sep 13, 2016

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 13, 2016

Member

@lmorchard: settings menu should only appear if the user has the add-on installed

Member

johngruen commented Sep 13, 2016

@lmorchard: settings menu should only appear if the user has the add-on installed

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 13, 2016

Member

settings menu should only appear if the user has the add-on installed

That's what I'm seeing... are you seeing something different?

Member

lmorchard commented Sep 13, 2016

settings menu should only appear if the user has the add-on installed

That's what I'm seeing... are you seeing something different?

@dannycoates

This comment has been minimized.

Show comment
Hide comment
@dannycoates

dannycoates Sep 13, 2016

Member

I also see settings without the addon

Member

dannycoates commented Sep 13, 2016

I also see settings without the addon

@ckprice

This comment has been minimized.

Show comment
Hide comment
@ckprice

ckprice Sep 13, 2016

Contributor

Can't see a couple GA events/pageviews firing

  • Viewing the home page without the add-on installed (ref).
  • Clicking Proceed on retiring modal and going to the retire page (ref).
  • Viewing the experiment detail pages (ref).

Eh, looks like there are no pageviews sends in the branch.

Think that's it.

/edit I'm putting together a commit for this.

Contributor

ckprice commented Sep 13, 2016

Can't see a couple GA events/pageviews firing

  • Viewing the home page without the add-on installed (ref).
  • Clicking Proceed on retiring modal and going to the retire page (ref).
  • Viewing the experiment detail pages (ref).

Eh, looks like there are no pageviews sends in the branch.

Think that's it.

/edit I'm putting together a commit for this.

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 13, 2016

Member

@dannycoates @lmorchard I wonder if this could be some kind of issue with the groundcontrol add-on. I can't repro on a clean profile

Member

johngruen commented Sep 13, 2016

@dannycoates @lmorchard I wonder if this could be some kind of issue with the groundcontrol add-on. I can't repro on a clean profile

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 13, 2016

Member

@lmorchard having a bit of trouble seeing the 'add-on installed' state on this branch

STR:

  • Create a new profile in dev edition
  • Turn xpi signatures off
  • go to localhost:8000
  • install the add-on
  • I see the secondary onboarding tab fire, but no redirect to /experiments
  • go to about:config
  • set the testpilot.env to local
  • refresh
  • still see the add-onless landing page when i refresh localhost:8000
Member

johngruen commented Sep 13, 2016

@lmorchard having a bit of trouble seeing the 'add-on installed' state on this branch

STR:

  • Create a new profile in dev edition
  • Turn xpi signatures off
  • go to localhost:8000
  • install the add-on
  • I see the secondary onboarding tab fire, but no redirect to /experiments
  • go to about:config
  • set the testpilot.env to local
  • refresh
  • still see the add-onless landing page when i refresh localhost:8000
@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 13, 2016

Member

@johngruen So, localhost:8000 won't work - you still have to use testpilot.dev:8000, because there's no recognized environment for localhost in the add-on.

Also, you have to set testpilot.env to local before you try installing the add-on. None of the events involved in telling the web frontend about add-on installation will work if that env is not set ahead of time.

All of this applies to both this React branch and the current Django master.

Member

lmorchard commented Sep 13, 2016

@johngruen So, localhost:8000 won't work - you still have to use testpilot.dev:8000, because there's no recognized environment for localhost in the add-on.

Also, you have to set testpilot.env to local before you try installing the add-on. None of the events involved in telling the web frontend about add-on installation will work if that env is not set ahead of time.

All of this applies to both this React branch and the current Django master.

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 13, 2016

Member

@lmorchard yeah that's what i figured, but when i go to testpilot.dev:8000 (w/ or w/o add-on) i get a timeout.

screen shot 2016-09-13 at 11 21 56 pm

Member

johngruen commented Sep 13, 2016

@lmorchard yeah that's what i figured, but when i go to testpilot.dev:8000 (w/ or w/o add-on) i get a timeout.

screen shot 2016-09-13 at 11 21 56 pm

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 13, 2016

Member

Are you running Docker at the same time? (shouldn't be) What does Gulp say when it starts up? At some point it should say "Server started http://0.0.0.0:8000". And also you should still have testpilot.dev in your /etc/hosts from Docker setup

Member

lmorchard commented Sep 13, 2016

Are you running Docker at the same time? (shouldn't be) What does Gulp say when it starts up? At some point it should say "Server started http://0.0.0.0:8000". And also you should still have testpilot.dev in your /etc/hosts from Docker setup

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 13, 2016

Member

@lmorchard yes to all:
docker not running, terminal reporting gulp server correctly...
in /etc/hosts i have
192.168.99.100 testpilot.dev

Member

johngruen commented Sep 13, 2016

@lmorchard yes to all:
docker not running, terminal reporting gulp server correctly...
in /etc/hosts i have
192.168.99.100 testpilot.dev

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 13, 2016

Member

yes to all, in /etc/hosts i have 192.168.99.100 testpilot.dev

Okay, that needs to be changed to:

127.0.0.1 testpilot.dev

We recently changed that when we moved the project to use Docker for Mac

Member

lmorchard commented Sep 13, 2016

yes to all, in /etc/hosts i have 192.168.99.100 testpilot.dev

Okay, that needs to be changed to:

127.0.0.1 testpilot.dev

We recently changed that when we moved the project to use Docker for Mac

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 13, 2016

Member

@lmorchard that did it. thank you!

Member

johngruen commented Sep 13, 2016

@lmorchard that did it. thank you!

@dannycoates

This comment has been minimized.

Show comment
Hide comment
@dannycoates

dannycoates Sep 13, 2016

Member

When a download fails the button remains in the enabling state.

screen shot 2016-09-13 at 2 54 14 pm

Member

dannycoates commented Sep 13, 2016

When a download fails the button remains in the enabling state.

screen shot 2016-09-13 at 2 54 14 pm

@dannycoates

This comment has been minimized.

Show comment
Hide comment
@dannycoates

dannycoates Sep 13, 2016

Member

When a download fails the button remains in the enabling state.

I get the feeling some of the things in this.state of ExperimentPage.js like isEnabling would be better as props managed by redux so that actions from outside can easily change them... @lmorchard @chuckharmston ?

Member

dannycoates commented Sep 13, 2016

When a download fails the button remains in the enabling state.

I get the feeling some of the things in this.state of ExperimentPage.js like isEnabling would be better as props managed by redux so that actions from outside can easily change them... @lmorchard @chuckharmston ?

@chuckharmston

This comment has been minimized.

Show comment
Hide comment
@chuckharmston

chuckharmston Sep 13, 2016

Contributor

@dannycoates What do you mean? Still haven't dug into this PR, but whether state or props (99% of the time if there's a question you should be using state*), you'd want to set up an event handler in this app that fires the action when told to by the add-on.

* That is to say, the connected components should be passing state down to children as props.

Contributor

chuckharmston commented Sep 13, 2016

@dannycoates What do you mean? Still haven't dug into this PR, but whether state or props (99% of the time if there's a question you should be using state*), you'd want to set up an event handler in this app that fires the action when told to by the add-on.

* That is to say, the connected components should be passing state down to children as props.

@dannycoates

This comment has been minimized.

Show comment
Hide comment
@dannycoates

dannycoates Sep 13, 2016

Member

What do you mean?

Sorry, my inexperience with React (jargon) is making it more difficult to communicate.

In this particular case take a look at the usage of isEnabling in ExperimentPage.js. Since its "local" to the Component when a download-failed message arrives from the addon it can't automatically update the ui. If isEnabling were somewhere in the redux store it seems to me that it'd be easier for the change to propagate.

I hope that made some sense

Member

dannycoates commented Sep 13, 2016

What do you mean?

Sorry, my inexperience with React (jargon) is making it more difficult to communicate.

In this particular case take a look at the usage of isEnabling in ExperimentPage.js. Since its "local" to the Component when a download-failed message arrives from the addon it can't automatically update the ui. If isEnabling were somewhere in the redux store it seems to me that it'd be easier for the change to propagate.

I hope that made some sense

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 14, 2016

Member

Hmm, there is an inProgress on experiments in the redux store. But, it looks like I didn't end up using it on the experiment page - I just use it as an index for finding the last experiment being installed, if I can't find it another way

But yeah, inProgress could be used to turn off local isEnabling / isDisabling state here. Or, it could make sense to actually move those two flags into the redux store

Member

lmorchard commented Sep 14, 2016

Hmm, there is an inProgress on experiments in the redux store. But, it looks like I didn't end up using it on the experiment page - I just use it as an index for finding the last experiment being installed, if I can't find it another way

But yeah, inProgress could be used to turn off local isEnabling / isDisabling state here. Or, it could make sense to actually move those two flags into the redux store

@johngruen

This comment has been minimized.

Show comment
Hide comment
@johngruen

johngruen Sep 14, 2016

Member

Here's one: experiments are sorted alphabetically, they should be sorted by experiment.id

Member

johngruen commented Sep 14, 2016

Here's one: experiments are sorted alphabetically, they should be sorted by experiment.id

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 14, 2016

Member

Here's one: experiments are sorted alphabetically, they should be sorted by experiment.id

Cool, that should be a one-liner to fix. The ID is a manual thing in YAML now, so we can use that for manual sorting.

Member

lmorchard commented Sep 14, 2016

Here's one: experiments are sorted alphabetically, they should be sorted by experiment.id

Cool, that should be a one-liner to fix. The ID is a manual thing in YAML now, so we can use that for manual sorting.

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

lmorchard added a commit that referenced this issue Sep 14, 2016

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Sep 14, 2016

Member

This is merged to master, now. 🎉

Let's file additional issues / submit PRs against master from here.

Member

lmorchard commented Sep 14, 2016

This is merged to master, now. 🎉

Let's file additional issues / submit PRs against master from here.

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