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

[env/burn-staging] add new AnimateMap venue template #1617

Closed
wants to merge 6 commits into from

Conversation

YarRainbow
Copy link
Contributor

  • The template was added following the example of the PartyMap.
  • Added dependencies to the package.json
  • Added venue reducer and another functions for work with redux-store
  • Added basic objects for upcoming works in the canvas.
  • Added vieports and quadtree for rendering objects
  • Cuted out all demo data

YarRainbow and others added 5 commits June 18, 2021 11:24
prepare structure for AnimateMap data
add settings for AnimateMap venue-item for creation venue page
add stub for AnimateMap venue template
add quadtree for visualization static venue
add LayerLOD with dynamic children
@YarRainbow YarRainbow requested review from a team as code owners June 21, 2021 19:18
@YarRainbow YarRainbow temporarily deployed to feature-preview June 21, 2021 19:18 Inactive
@github-actions github-actions bot added dependencies Pull requests that update a dependency file 💎 styles For (S)CSS style related issues/changes/improvements/etc. labels Jun 21, 2021
@YarRainbow YarRainbow changed the base branch from staging to env/burn-staging June 21, 2021 19:18
@codeclimate
Copy link

codeclimate bot commented Jun 21, 2021

Code Climate has analyzed commit 6340eae and detected 18 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 7
Duplication 11

View more on Code Climate.

@github-actions
Copy link

Visit the preview URL for this PR (updated for commit 6340eae):

https://co-reality-staging--preview-pr-1617-z2e7kshr.web.app

(expires Mon, 05 Jul 2021 19:22:58 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@0xdevalias
Copy link
Contributor

0xdevalias commented Jun 22, 2021

@YarRainbow Out of curiosity, was this PR originally opened targeting staging/similar? I'm curious why #1617 (comment) ran on this PR, as I don't think it should have currently based on the trigger rules in our workflow (ref). So trying to understand what happened with our systems here. If it was opened against staging then re-targeted then this makes sense. If not.. then I have no idea what our systems are doing.. so any extra info/data you can provide to help debug/understand that would be useful. (cc // @sunny-viktoryia @jonboutelle FYI)

@YarRainbow
Copy link
Contributor Author

YarRainbow commented Jun 22, 2021

@0xdevalias sorry, I was late for the train and was in a hurry. All our branches forked from env/burn-staging and target on this branch.
If I broke some github actions, you can reopen this PR correctly and close broken PR. I will be grateful to you.
I will be available after 5pm (GMT+10)

Copy link
Contributor

@0xdevalias 0xdevalias left a comment

Choose a reason for hiding this comment

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

Gave this a quick pass over to leave some comments/suggestions/pointers to help you align with how we do things, our codebase style, etc.

Learning about these things early on, and embedding them in your workflow will allow you to write code aligned to our standards from the very beginning, and will reduce the need for extra work/frustration/etc to try and align it later on.

Please share this among the rest of your engineering/dev team to ensure that everyone learns/benefits from the knowledge shared here.

Thanks.

(cc // @jonboutelle @sunny-viktoryia @mike-lvov FYI)

return (
<div ref={containerRef} className="animatemap-venue-container">
{container && (
<ReactReduxContext.Consumer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the redux hooks-based API: https://react-redux.js.org/api/hooks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xdevalias I consulted with Vasily about his decision. We are facing a known issue with react-pixi. We only use this wrapper to patch the provider. And then we use only hooks according to your recommendations.
We can encapsulate the wrapper in a separate component, but we don't see a way to get rid of it entirely.
If you have an idea how to fix this, I will be glad to consider it.
PS pixijs/pixi-react#77

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for the context. Following some of those links I ended up at this section of the README:

This links to the following React issue:

Which seems to have been closed by this PR:

Which seems to have been included in the React 16.6.0 changelog in this PR:

Since we seem to be using 16.13.1, my guess/theory is that we should have that fix available to us:

I'm not really sure of the specifics of how this issue works, how it was fixed, or how that applies to the lib you're using here, but it would be good if you could quickly see if it is resolved in a 'proper' way now, and if so, align the code here to that 'proper' way.

If it's going to take too long/be too hard/etc, then can you add the following comment (replacing the XXX's with the relevant details) just before the return JSX so we have a note captured for future:

// @debt We need to use Redux's XXX here to work around a limitation in react-pixi-fibre as per:
//   https://github.com/michalochman/react-pixi-fiber#react-context-api-limitations
//
// While this issue was resolved in React in v16.6.0 via facebook/react#13728, we can't use that
//   fix here yet because XXX. Once that is resolved, we should refactor this accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if we still do need to hack around it like this, I wonder if we can still simplify/move the code here to a 'closer to Redux hooks' based implementation by making use of useStore instead of ReactReduxContext.Consumer, etc:

src/components/templates/AnimateMap/AnimateMap.tsx Outdated Show resolved Hide resolved
src/components/templates/AnimateMap/AnimateMap.tsx Outdated Show resolved Hide resolved
src/components/templates/AnimateMap/AnimateMap.scss Outdated Show resolved Hide resolved
src/components/templates/AnimateMap/AnimateMap.scss Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
src/components/templates/AnimateMap/AnimateMap.tsx Outdated Show resolved Hide resolved
Comment on lines +44 to +46
<Provider store={store}>
{app ? <LoadingContainer /> : <Container />}
</Provider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the redux hooks-based API: https://react-redux.js.org/api/hooks

Copy link
Contributor

@0xdevalias 0xdevalias Jun 22, 2021

Choose a reason for hiding this comment

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

See this for potentially related reason why this isn't currently using hooks: #1617 (comment)

src/components/templates/AnimateMap/AnimateMap.tsx Outdated Show resolved Hide resolved
@0xdevalias 0xdevalias changed the title Beaver's Animate Map template [env/burn-staging] add new AnimateMap venue template Jun 22, 2021
@0xdevalias 0xdevalias added the 💡 new-feature New idea/feature request label Jun 22, 2021
@0xdevalias
Copy link
Contributor

0xdevalias commented Jun 22, 2021

RE: #1617 (comment)

sorry, I was late for the train and was in a hurry. All our branches forked from env/burn-staging and target on this branch.
If this broken some github actions, you can reopen this correctly and close broken PR. I will be grateful to you.

@YarRainbow All good, didn't 'break anything' at all, more just confused me why that workflow ran. Thanks for letting me know the process you followed.. sounds like it might be something weird going on in our actions workflow then (might not be anything you've done 'wrong' with this PR at all). I'll make a mental note to look into it more later on and see if I can figure out the 'why' of it. Not something you need to look into. Thanks for the insight! :)

For reference though, you probably shouldn't use that preview link at all, as it's connected to our main staging environment, not env/burn-staging, so will connect to the wrong firebase project/etc.

rename CSS classes, remove useless styles
remove deprecated PIXI imports
remove related path for some modules
remove Object.Assign
return tsconfig.json
@YarRainbow YarRainbow closed this Aug 5, 2021
@YarRainbow YarRainbow deleted the beavers/animate-map branch August 5, 2021 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file 💡 new-feature New idea/feature request 💎 styles For (S)CSS style related issues/changes/improvements/etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants