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

Clean-up Flexbox Layout algorithm implementation #39

Closed
NickAcPT opened this issue May 13, 2022 · 5 comments
Closed

Clean-up Flexbox Layout algorithm implementation #39

NickAcPT opened this issue May 13, 2022 · 5 comments
Labels
code quality Make the code cleaner or prettier. enhancement New feature or request good first issue Good for newcomers

Comments

@NickAcPT
Copy link

NickAcPT commented May 13, 2022

Problem

While browsing the implementation of the algorithm in this project, I noticed some things that probably should be cleaned up in the future.

Missing Implementation

There's a few steps that are not implemented yet and probably should be.

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L296-L302

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L304-L310

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L304-L310

Out of spec Implementation

It seems like there's some steps in the implemented algorithm that are outside of the Flexbox specification.

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L269-L270

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L360-L363

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L454-L455

https://github.com/DioxusLabs/stretch/blob/a50a4569c5dbcebdcb77408f804d9bf90ccdc508/src/algo.rs#L605-L609

Tight coupling

Maintainability of the Flexbox algorithm in the future is important, more so if more algorithms are implemented side-by-side next to it.
Thus, I think that the flexbox (and future) algorithm implementation(s) should be divided by steps.
While it requires more code to setup and do properly, it will allow to have simpler and shorter methods for each step of the layouting.

I hope I didn't miss anything, first time I am opening an issue here.

@TimJentzsch TimJentzsch added enhancement New feature or request code quality Make the code cleaner or prettier. labels May 13, 2022
@alice-i-cecile
Copy link
Collaborator

This is a fantastic issue, and I agree on all of the points you raised.

The only other suggestions I have to implementors is a) do the refactor in one PR, then add the other steps in a second PR b) remember to double-check benchmarks to avoid major regression and c) remember to use the #[inline] attribute macro on your little functions to help the compiler optimize this.

@alice-i-cecile alice-i-cecile added the good first issue Good for newcomers label May 13, 2022
@TimJentzsch
Copy link
Collaborator

I suggest that we split this issue up as two as well, for the two steps you suggested.
Then it's easier to keep track and to assign the tasks to contributors.

@TimJentzsch
Copy link
Collaborator

I think now that the refactoring is merged, it would be a good idea to split this up into multiple smaller, actionable issues for each thing that needs to be improved.

This would make it easier to implement the features, review them and to keep progress of what needs to be done.

@alice-i-cecile
Copy link
Collaborator

Agreed. Closing; please do make scoped follow-up issues!

@TimJentzsch
Copy link
Collaborator

Not sure if I got all of them, but probably most.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Make the code cleaner or prettier. enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants