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

FE: Online partition count is red fix #137

Merged
merged 9 commits into from
Feb 28, 2024
Merged

FE: Online partition count is red fix #137

merged 9 commits into from
Feb 28, 2024

Conversation

Leshe4ka
Copy link
Contributor

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

@Leshe4ka Leshe4ka added scope/frontend Related to frontend changes type/bug Something isn't working labels Feb 14, 2024
@Leshe4ka Leshe4ka self-assigned this Feb 14, 2024
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels Feb 14, 2024
@Leshe4ka Leshe4ka requested review from Vixtir and removed request for Mgrdich February 14, 2024 14:32
@Leshe4ka Leshe4ka linked an issue Feb 14, 2024 that may be closed by this pull request
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there Leshe4ka! 👋

Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖

We will try to review it soon!

@Leshe4ka Leshe4ka requested review from Mgrdich and Vixtir and removed request for Vixtir and Mgrdich February 14, 2024 14:33
@Mgrdich Mgrdich requested review from Mgrdich and removed request for Vixtir February 18, 2024 21:48
@Leshe4ka
Copy link
Contributor Author

@Mgrdich approve, please, if you haven't got questions more. My next task relates to this part of code with Brokers and I don't want a lot of conflicts.

@Mgrdich
Copy link
Contributor

Mgrdich commented Feb 25, 2024

It looks good, but SizeCell still has the any typing instead of a generic

@Leshe4ka
Copy link
Contributor Author

Leshe4ka commented Feb 25, 2024

@Mgrdich I have rollbacked SizeCell to unknown as you wish)))

Let's look at the errors related to the table (where the others in main came from - I don't know).
And I found an interesting peculiarity. Now in the table for the columns field there is a generic TValue, which breaks the typing and excludes the possibility of processing unions for the column value. You can notice this error in the BrokersList.tsx file . And it is not about undefined type. Even if you take the BrokersTableRow type of this kind for checking, TS will start complaining to you that Type number is not assignable to type string. Although this should not be the case.

Then why did you need a generic TValue in the Table component? Taking into account that it is not used anywhere in the code of the component itself, but only thrown into props. Moreover, if we look at the typing of the tanstack/react-table library itself, we will see that the developers of the library themselves have strictly typed the columns parameter to be passed to useReactTable. And there it is typed as columns: ColumnDef<TData, any>[]. That is, even the library creators themselves do not accept column value as a generic. And we can't do it even if we really wish we could.

Why this problem was not noticed earlier in other code places? Because in all places where table columns were formed, either an explicit generic was thrown into getValue, or unknown was thrown into CellContext instead of TValue.... That is, in fact, there was never a strict TValue typing.

My proposed scheme of table composition assumes strict typing of components used in the cell field, strict typing of the result returned by the getValue function (not at the place of use, but at the stage of table generation, automatically).To do this, we just need to remove the already unused generic TValue from the implementation of the NewTable component. What do you think?

@Mgrdich
Copy link
Contributor

Mgrdich commented Feb 25, 2024

i think i intercommunicated , i will take a look why the typescript is breaking in your branch.

we needed the generic to make the Table component full generic , so it can infer the correct types base on the table data.

@Leshe4ka
Copy link
Contributor Author

No problem, please take a look. It is not necessary to use a generic TValue in ColumnDef in a table component. It is enough to strictly describe the columns at the stage of their creation, and the default tanstack table tools will correctly pick up the column types

@Leshe4ka Leshe4ka merged commit 481df49 into main Feb 28, 2024
0 of 6 checks passed
@Leshe4ka Leshe4ka deleted the issues/26 branch February 28, 2024 04:38
@Haarolean Haarolean removed the status/triage/manual Manual triage in progress label Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope/frontend Related to frontend changes status/triage/completed Automatic triage completed type/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Online partition count is red
3 participants