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

moderate comments UI #25

Merged
merged 8 commits into from
Oct 24, 2024
Merged

moderate comments UI #25

merged 8 commits into from
Oct 24, 2024

Conversation

thomassth
Copy link
Contributor

@thomassth thomassth commented Oct 23, 2024

issues:

  • the storybook-prep branch deviates more and more, making dev effort harder
  • local storybook still has redux error, even though the online pipeline has no issue

@patcon
Copy link
Member

patcon commented Oct 23, 2024

Yay thanks!

  1. I'm def not saying we shouldn't use this for components in our own forked codebase, but just suggesting that the way to do it is to add a new git submodule in codebases/CivicTechTO, rather than diverging 'codebases/compdem` from anything but the bare minimum changes to get their stories mounted. Just like how UT-HAI has its own :)
  2. Yeah, we're trying to cram a 3 apps worth of packages into one package.json dependency resolution, so it takes some massaging of package versions, and the tactic might have breaking points that we can't resolve without creating 3 separate storybook sites. Just trying to avoid that if we can, and take the right tradeoffs for simplicity!

@patcon
Copy link
Member

patcon commented Oct 23, 2024

I can review this, but you're also welcome to stub out a PR to add our forked codebase, and we can start putting stories there. We can either (1) leave it mostly empty except when mounting our stories that differ significantly (due to our changes, (b) import/export the same stories from other codebases, so we have a full set, and no code duplicates of stories

Thoughts?

@thomassth
Copy link
Contributor Author

I believe we're almost at break point, where (old) packages start to clash with each other. Namely, 2 components are clashing hard:

  • radium
  • react-chat-widget (exclusive in UT-HAI)

@thomassth
Copy link
Contributor Author

thomassth commented Oct 24, 2024

I'd suggest merging all the patches for storybook (if reasonable) into polis, since they are modernizing the components anyways.

CivicTechTO/polis#56

@patcon
Copy link
Member

patcon commented Oct 24, 2024

since they are modernizing the components anyways

I love your optimism, but my preference is to always wait and never get ahead of the upstream project (again, this is ONLY for the codebases/compdem, which is specifically claiming to be tracking upstream).

We can create our own git submodule and do pretty much whatever we'd like, which I'm suggesting we do :)

Reviewing this PR now tho!

@patcon
Copy link
Member

patcon commented Oct 24, 2024

Yay! Ok, added some extra commits to get this ready! Thanks for starting on it!

the error wasn't due to redux versions, but because the data being passed to the store as initialState was incorrect (arrays instead of objects). I redid the withRedux decorator to allow passing in arbitarry state in stories via storybook parameter, and fixed the issue

I also downgrade redux to match the very in client-admin in codebases/compdem, and hopefully that loosens the constraint you mentioned with the chat package.

I also removed d3 because I wasn't sure why it was there, but maybe you could help me understand that bit, if it's needed? Ah, seeing the build error, but I think that's something else. Lemme investigate :)

As far as I can tell, there is still no need to update the submodule to edge-civictechto. Are you ok with my reverting that back to storybook-prep?

@patcon
Copy link
Member

patcon commented Oct 24, 2024

Ah, seeing the build error, but I think that's something else. Lemme investigate :)

Ok, so that error is because imports from the whole d3 project have been added in our fork. If that's fixed to pull directly from sub-packages, the issue will go away. See here for how that can be done:

import * as d3array from 'd3-array';
import * as d3force from 'd3-force';
import * as d3geo from 'd3-geo';
import * as d3scale from 'd3-scale';
import * as d3voronoi from 'd3-voronoi';
global.d3 = {
...d3array,
...d3force,
...d3geo,
...d3scale,
...d3voronoi,
};

(but you're just do it for const d3, not a global)

I'm going to revert to storybook-prep to get this working and ready to merge. Thanks a million @thomassth 🎉

@patcon
Copy link
Member

patcon commented Oct 24, 2024

To clarify, I don't think any of the challenges here had anything to do with storybook-prep. Just from accidentally passing malformed data into the redux store being initialized, and also from something happening in edge-civictechto that isn't compatible with the aggressive tree-shaking we're supporting in some custom components here (ie. never expecting full d3 imports)

@patcon
Copy link
Member

patcon commented Oct 24, 2024

Working! yaaaay https://civictechto.github.io/polis-storybook/PR-25/?path=/story/compdem-client-admin-moderatecommentstodo--default

Merge at will, or lemme know if you think it need more time!

@thomassth
Copy link
Contributor Author

I really struggled with the git-in-git management. Thanks for the help!

@patcon
Copy link
Member

patcon commented Oct 24, 2024

Heh yeah, you're not alone: submodules are notorious for making hit workflows confusing...!

@thomassth thomassth merged commit 6216fcb into main Oct 24, 2024
2 checks passed
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.

2 participants