Extract match detail components - #1345
Conversation
…l-components # Conflicts: # lib/teiserver_web/live/battles/match/show.html.heex
|
@Zonkodonko I saw you closed then re-opened this PR. I don't want to review it before you are ready so feel free to ping me whenever you are happy for a review. |
Teifion
left a comment
Teifion
left a comment
There was a problem hiding this comment.
This is a nice first step to break the page up. Aside from the request for a test case for the different dropdown options I would like to see the components moved to the lib/teiserver_web/components/match_components path and appropriate module renaming to go with it please.
Once those two are done I think it should be an easy approve; thank you.
|
|
||
| ~H""" | ||
| <form method="post" class=""> | ||
| <.input |
There was a problem hiding this comment.
Selecting "default" on this locally causes an error.
14:39:28.946 [error] GenServer #PID<0.8240.0> terminating
** (RuntimeError) No balance module by the name of 'default'
(teiserver 0.1.0) lib/teiserver/battle/libs/balance_lib.ex:179: Teiserver.Battle.BalanceLib.create_balance/3
(teiserver 0.1.0) lib/teiserver_web/live/battles/match/sub_components/balance_component.ex:48: TeiserverWeb.Battle.MatchLive.SubComponents.BalanceComponent."balance_tab (overridable 1)"/1
It does so on the main site so I would like you to remove that as an option and add a test case for the options that exist as part of the refactor please.
There was a problem hiding this comment.
@Teifion Im planning to build a "parent" component for battles which includes nav button which switch out those sub components via a live view. For this purpose I would like to keep all components that are part of this "battle" component in a dedicated package route that referneces to this planned structure.
How would you feel about placing them in something like lib/teiserver_web/live/battle/views (views are gonna be the different sub components) ?
See:
The big yellow box on the right is gonna be the live view which will host those sub components
There was a problem hiding this comment.
Views is not an appropriate name for it, they're components so we will be keeping that nomenclature and I want them to stay within the components folder please.
You can have components use other components without them having to be in a sub folder. Additionally for smaller components without logic (e.g. menu bars) you can have multiple in a module similar to the CoreComponents and NavComponents we already have.
There was a problem hiding this comment.
Why do you want sub components which we will only be using inside a specific component on a high level package? I get that for smaller components which can be used on multiple pages you would place those in a high level package because they are gonna be used all over the app. But I think its rather confusing if you dont put dedicated sub components and their parents inside the same package route. They are much easier to find this way.
We dont have to call them views(even if they are only used as content in a live view) but I insist that they stay together with their parent component as long as there isn't any elixir convention that I don't know about which would be a good reason to put them in a completly unrelated package.
There was a problem hiding this comment.
If the item will only ever be used once and has no logic to it then it shouldn't be turned into a component. If it will be used multiple times (e.g. navbars) or has logic (e.g. forms or dynamic interfaces) it goes into a component and components should live in that directory as mentioned.
The convention is a combination of Phoenix and Elixir.
There was a problem hiding this comment.
Selecting "default" on this locally causes an error.
14:39:28.946 [error] GenServer #PID<0.8240.0> terminating ** (RuntimeError) No balance module by the name of 'default' (teiserver 0.1.0) lib/teiserver/battle/libs/balance_lib.ex:179: Teiserver.Battle.BalanceLib.create_balance/3 (teiserver 0.1.0) lib/teiserver_web/live/battles/match/sub_components/balance_component.ex:48: TeiserverWeb.Battle.MatchLive.SubComponents.BalanceComponent."balance_tab (overridable 1)"/1It does so on the main site so I would like you to remove that as an option and add a test case for the options that exist as part of the refactor please.
This error did not get introduced by Zonko. It is already present in beyond-all-reason:main
There was a problem hiding this comment.
Thanks @ChocoboCoding, that's a flakey test and I can see it wasn't introduced as part of this PR.
3 participants
In preparation to the new match page design. I extracted all tabs of the match details page to single component files.
I also removed the unused columns from matchdetails.