search

CivicTechTO / polis-storybook

https://civictechto.github.io/polis-storybook/
GNU Affero General Public License v3.0
1 stars 1 forks source link

moderate comments UI #25

Closed thomassth closed 1 month ago

thomassth commented 1 month ago

issues:

patcon commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

thomassth commented 1 month ago

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

https://github.com/CivicTechTO/polis/pull/56

patcon commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

https://github.com/CivicTechTO/polis-storybook/blob/444a0601db6b850baf2e5802d397fca3a2f08129/.storybook/preview.js#L10-L21

(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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

patcon commented 1 month ago

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