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

Remove lib/products-list #22901

Closed
wants to merge 13 commits into from
Closed

Remove lib/products-list #22901

wants to merge 13 commits into from

Conversation

spen
Copy link
Contributor

@spen spen commented Mar 1, 2018

Note: This ended up being a huge chunk of work. I'm happy to cherry-pick out commits if that would help reviewing - just let me know :)

Summary

While I was in the air ✈️ I wanted to do some janitorial work on the project. One thing I'm always happy to do is 🔪mixin usage and anything fluxxy!

The rabbit hole I chose to dive in to on this trip was to reduce some usage of the observe mixin, in this case by removing products-list.

We we're already half way there, with products-list already having a full set of redux reducers and actions so it was more a case of plugging wires in to the right places.

Testing

Of course, everything should work exactly as before. I foresee the biggest pain point being in fetching/sync timing. The flux store had a handy getter that would fetch data where needed, something we don't have a direct mirror of with redux (the issue is solved by dispatching a fetch action via a query component)... So please be very vigilant about this point and assume that I've missed something, somewhere.

Run the full unit test suite

  • run: npm run test-client - there should of course be no regressions that cause any tests to fail.

Domain Management

Upgrades - Google Apps

Coming...

Signup - Domains

  • Either log out, or open a private browsing session.
  • Go through the forms, paying close attention to the domain step.
  • Try using the search functionality - it should still work.
  • Try selecting a domain - that should also still work.

Signup - Cart

Currently not working
At the point at which the cart is built with items from signup (domain, plan etc.) the products have not been fetched or stored in redux state. The fix may be quite involved as this needs to be done via the dispatcher and needs to be synchronous (We need those products in redux before building the cart).

Checkout

  • Thank You
    Coming...
  • Privacy Protection
    Coming...

This is a work in progress and isn't something I can dedicate a big chunk of my own time to. I'll get there eventually but if anybody is able to help iron out the remaining issues and get a fully successful run through the complete test plan I would appreciate that very much!

I've added to the 'Reduxify Flux: One Project to Rule Them All' project board, feel free to remove if I've done so wrongly.

Seems to relate to: #5046 & #6709

@matticbot
Copy link
Contributor

@spen spen added the [Feature Group] Emails & Domains Features related to email integrations and domain management. label Mar 1, 2018
@blowery blowery mentioned this pull request Apr 12, 2018
@spen spen force-pushed the try/remove-lib-products-list branch from e71c9bf to e1dfec6 Compare April 14, 2018 16:53
@spen spen requested a review from aidvu April 14, 2018 16:54
@spen
Copy link
Contributor Author

spen commented Apr 14, 2018

Rebased.
Tagging @aidvu as there was some overlapping with some of your recent work in the rebase, want to make sure I've not undone any of your work or broke anything subtly :)

@spen spen force-pushed the try/remove-lib-products-list branch from e1dfec6 to 259ef99 Compare April 14, 2018 20:18
@aidvu aidvu requested a review from a team April 16, 2018 20:30
@alisterscott
Copy link
Contributor

This branch needs a rebase. Is this PR still required?

@spen
Copy link
Contributor Author

spen commented Jul 24, 2018

I believe this PR was superseded by other PRs 👌

@spen spen closed this Jul 24, 2018
@alisterscott alisterscott deleted the try/remove-lib-products-list branch August 8, 2018 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants