Skip to content

Consider removing Dimension::Undefined #114

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.

Sign up for GitHub

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

Jump to bottom
Closed
alice-i-cecile opened this issue Jun 9, 2022 · 3 comments
Closed

Consider removing Dimension::Undefined #114

alice-i-cecile opened this issue Jun 9, 2022 · 3 comments
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier. good first issue Good for newcomers

Comments

@alice-i-cecile
Copy link
Collaborator

My instincts suggest this should be an Option<Dimension>.
@alice-i-cecile alice-i-cecile added code quality Make the code cleaner or prettier. breaking-change A change that breaks our public interface labels Jun 9, 2022
@Weibye Weibye mentioned this issue Jun 11, 2022
Closed
@alice-i-cecile alice-i-cecile added the good first issue Good for newcomers label Jun 12, 2022
@Weibye Weibye mentioned this issue Jun 12, 2022
Closed
@Weibye Weibye mentioned this issue Jun 23, 2022
Closed
@nicoburns
Copy link
Collaborator

@alice-i-cecile @Weibye I might be being stupid, but doesn't this change (replacing Dimension::Undefined with Option) effectively undo the earlier change of making Dimension::Auto the default dimension? If this change is made, then the default will now effectively be Option::None (= Dimension::Undefined).

@Weibye
Copy link
Collaborator

Weibye commented Jul 18, 2022

Good thing to flag but I think it should be solved by having the default FleboxLayout values be Some(Dimension::Auto), and every variable / parameter that previously used Dimension::Undefined should now have the Option<Dimension> type.

You can see the current state of this over at #188 (and please leave a review)

It could be that changing these default values is what is causing the tests to fail. I haven't been able to get to the bottom of that yet so extra eyes would be appreciated :)

@nicoburns nicoburns mentioned this issue Nov 28, 2022
Merged
@nicoburns
Copy link
Collaborator

Implemented in #269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier. good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
3 participants
@nicoburns @alice-i-cecile @Weibye