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

Projects: move panel mgmt to React Context #1010

Merged
merged 21 commits into from
Jul 9, 2019
Merged

Conversation

chadoh
Copy link
Collaborator

@chadoh chadoh commented Jun 19, 2019

Fixes #978

This change slims down the App.js file in Projects by hundreds of lines, primarily by pushing logic to hooks and context.

Reviewing

@coveralls
Copy link

coveralls commented Jun 19, 2019

Pull Request Test Coverage Report for Build 2470

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2465: 0.0%
Covered Lines: 311
Relevant Lines: 311

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 19, 2019

Pull Request Test Coverage Report for Build 2473

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2465: 0.0%
Covered Lines: 311
Relevant Lines: 311

💛 - Coveralls

@chadoh chadoh force-pushed the 978-slimmer-appjs-with-context branch 5 times, most recently from 353589e to 900c1b9 Compare June 24, 2019 18:54
@chadoh chadoh force-pushed the 978-slimmer-appjs-with-context branch from 900c1b9 to 6166834 Compare June 24, 2019 19:19
@chadoh chadoh changed the base branch from dev to simplify-NewProject June 24, 2019 19:22
@chadoh chadoh force-pushed the 978-slimmer-appjs-with-context branch 5 times, most recently from e0c6e87 to fb2b5c0 Compare June 25, 2019 20:46
@chadoh chadoh changed the base branch from simplify-NewProject to dev June 25, 2019 20:49
@chadoh chadoh changed the base branch from dev to simplify-NewProject June 25, 2019 20:50
@chadoh chadoh marked this pull request as ready for review June 25, 2019 21:06
@chadoh chadoh force-pushed the 978-slimmer-appjs-with-context branch 2 times, most recently from 713053f to e0ded45 Compare June 26, 2019 02:10
@ottodevs
Copy link
Member

@chadoh the PR status is "In progress" is that accurate, or is it ready for review?

@chadoh
Copy link
Collaborator Author

chadoh commented Jun 26, 2019 via email

@chadoh chadoh force-pushed the 978-slimmer-appjs-with-context branch 2 times, most recently from 4cb47a3 to 4b6d13b Compare June 26, 2019 18:31
chadoh added 20 commits July 5, 2019 15:14
BountyContextMenu doesn't want to know about the specifics of opening a
panel. It just wants to call a function.

This locates the logic for how to open a specific panel next to the
PanelManager component. This gives us a clean place to which to relocate
other panel-opening logic, rather than putting it all in App.js.
This new functional component can use React hooks, which will allow us
to move `curateIssues` and `allocateBounties` to the
`usePanelManagement` custom hook.
This makes it easier to move panel-opening logic to the
`usePanelManagement` hook, since these panels no longer rely on state
from App.js
This removes all reliance that FundIssues had on props from App.js,
so that logic for opening this panel can move out of App.js entirely
This allows moving opening of the panel to usePanelManagement hook
* Open these panels using the `usePanelManagement` hook
* Clean up `ActionsMenu.propTypes`
* Simplify `Issues` somewhat
Perplexingly, without the change to `LoadingAnimation` to render an
inline svg, this resulted in cryptic Parcel errors like:

> Uncaught (in promise) Error: Cannot find module 'components/Shared/assets/svg/ethereum-loading.svg'
>    at newRequire (VM856 app.e31bb0bc.js:37)

Another way to fix this issue is to remove the dynamic imports from
`PanelManager`.

After hours of research, I'm not sure how the rest of this change
resulted in such an error, but I'm moving on for now.
It is now `editBounty`, since it opens a panel to edit the bounty,
rather than actually sending the update request.
It only uses workStatus, which comes from `issue`

Also,

* when rendering Issues, use their id as `key`, instead of the order in
  the list
I would prefer to use React.memo with the functional component, but am
preferring PureComponent for the sake of reducing noise in the pull
request.
The individual files that need props from useAragonApi are now
responsible for calling it. This keeps the code close to where it's used
and makes it easier to understand where props are coming from.
@chadoh chadoh force-pushed the 978-slimmer-appjs-with-context branch from 9f89cb0 to 1c7343b Compare July 5, 2019 19:14
@chadoh chadoh requested review from ottodevs and Quazia July 5, 2019 19:14
@stellarmagnet stellarmagnet added this to the Sprint 19.17 milestone Jul 8, 2019
@Quazia
Copy link
Collaborator

Quazia commented Jul 8, 2019

I agree! That would be great. A quick search for how to automate such "performance regression checks" in our CI pipeline led me to a blog post about using Lighthouse and Circle CI. I wonder if it makes to wait until we get our e2e testing setup merged so we can explore Cypress a bit more, and see if there's a way to integrate Lighthouse or another performance-checker with our Cypress setup.
This sort of research could easily take multiple days, though I think it would be worth it in the long-run. Want me to go ahead and make a ticket for it now?

I think capture a ticket and we can start looking into this post mainnet.

Copy link
Member

@ottodevs ottodevs left a comment

Choose a reason for hiding this comment

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

In my recent tests with latest codebase the most concerning issues were addressed, and overall the code works good.

A lot of code was cleaned and the code changes seem ok

I have some other findings, probably most of them are not really related, and I think this PR is already huge, just to take note of these to capture on other tickets if relevant:

  • Basically once we stop using some complex react lifecycles we could change lot of other components to be functional, like App.js and the panels (most of them have right now the TODO note about that)
  • We can still abstract lot of duplicated logic to hooks to reuse the code a lot, specially the panels seem to have a lot of duplicated boilerplate
  • We could encode the proptypes to clean the code a lot, specially the complex objects that need shape
  • Lot of props seem not properly typed (I am pretty sure @chadoh addressed this on other commits)
  • View funding proposal could probably be improved a bit, probably out of scope but something to take in mind, some data will need additional Apollo queries other can be taken as we do on issuedetail
  • Lot of issues transformations happen on the render function, that should be done IMHO in the reducer and some others just Apollo requests, taking advantage of the nice zero config caching it offers (like array of issue names or things like that, so we don't need to manipulate some data locally or we could save some of those transformations)
  • When removing repos or creating new issues there is no visual feedback at all, this should happen without the need to refresh
  • Filter bar is not correctly showing the filtered results, for example for funded issues, specially after a cache clean
  • We want to adopt a solution like aragon/ui has to do the svg cleaning and minification by using a script instead of in-lining svg code from other files (ethereum loading animation) - that is probably not a priority but nice to have and would save time in the mid/long term
  • Some things like the github authenticated user should not be loaded each time we visit the settings tab, but be already loaded in a more global state and only read by the settings field
  • I generally rather to assign default values for props into the beginning of the components instead of dealing with conditionals in the render function (thing like token={props.token || []} would be cleaner if we deal with default values for token just when the prop is received, maybe this is more opinionated, but I find this more readable and cleaner)
  • The activity log is wiped when a funding is rejected and a new work submission starts

If you @chadoh or @Quazia think any of these are in the scope of this issue would be nice to incorporate, but as I said I encourage to put these on new tickets and merge this one ASAP :)

Nice job @chadoh !

@ottodevs
Copy link
Member

ottodevs commented Jul 8, 2019

I also commented with @chadoh about the -not that much annoying- 401 unauthorized errors that we experienced on other branches as well, the issue is with the useGithubAuth() hook, line 17, maybe if we put a try-catch to handle that promise result we would have more control over the response, the same as my other comment, if we decide this is in the scope of this PR let's do it, should be trivial to add, otherwise I can capture the issue outside

@chadoh chadoh merged commit 07c3b0f into dev Jul 9, 2019
@chadoh chadoh deleted the 978-slimmer-appjs-with-context branch July 9, 2019 02:45
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.

Projects: Move Panel management to React Context
5 participants