search

merisbahti / klyva

A state management library that follows the React component model
MIT License
55 stars 4 forks source link

TodoMVC example reorganisation #23

Closed krawaller closed 3 years ago

krawaller commented 3 years ago

Still no tests, just moving stuff around in anticipation of adding tests.

Also: I'm not 100% sure this is a good idea, so please feel free to shoot this PR down!

image

merisbahti commented 3 years ago

Going from a flat components folder, to this organised tree is in theory nice - the only argument against it is that I find this kind of organisation hard to maintain. Now, this app probably won't grow any new verticals anytime soon, so it should be fine!! So everything that I've said up to now is sum positive I think!

Just 1 thing, it's a bit confusing that there's a "HeaderSection", "MainSection", and an "AppFooter" - but the "AppFooter" is inside "MainSection". Shouldn't "AppFooter" be outside with "HeaderSection" And "MainSection"?

But I guess it's completely logical if the file structure is "colocated", i.e. that all components that are under the component in the react tree, are under the component in file system tree.

Again, nothing to act upon here. I think it's fine.

Just 1 nitpick:

Instead of for example: AppFooter/AppFooter.tsxand a AppFooter/index.tsx, we could have only a AppFooter/index.tsx with the exact contents of AppFooter/AppFooter.tsx.

It's kinda implicit that the if something is named index in the folder of AppFooter, that it's the AppFooter being exported.

krawaller commented 3 years ago

Shouldn't "AppFooter" be outside with "HeaderSection" And "MainSection"?

So, I was about to reply that this is because of a "bug" with the TodoMVC CSS. The HTML structure has to look like this:

<section class="todoapp">
  <header class="header">...</header>
  <section class="main">
    <input /><label />
    <ul class="todo-list">...</ul>
    <footer class="footer"></footer>
  </section>
</section>

In other words, <footer class="footer"> must be located INSIDE <section class="main"> for things to look correct.

BUT! I just tried to move it out where it should more intiuitively live, as you suggested, and - lo and behold - it works!

Either...

  1. the bug was fixed
  2. I was confused way back when I "discovered" the bug
  3. I'm misremembering.

Alternative 2 and 3 both have high likelyhood. :D

krawaller commented 3 years ago

@merisbahti Just revisited these changes. While the flat->tree reorg is not objectively better, the other changes in this PR (context, split ui-data, etc) I think are a good improvement. So if the tree structure isn't a blocker - and I don't think it is, it works fine enough - I humbly vote for merging this!