Skip to content

Moving flexbox behind a feature flag#414

Merged
nicoburns merged 20 commits intoDioxusLabs:mainfrom
Weibye:feat/flexbox-feature
Apr 10, 2023
Merged

Moving flexbox behind a feature flag#414
nicoburns merged 20 commits intoDioxusLabs:mainfrom
Weibye:feat/flexbox-feature

Conversation

@Weibye
Copy link
Copy Markdown
Collaborator

@Weibye Weibye commented Apr 7, 2023 •
edited
Loading

Objective

Why did you make this PR?

Fix #298

Feedback wanted

  • Does it make sense that Display::None is the default value for the node?

Weibye
Weibye commented Apr 7, 2023
View reviewed changes
Weibye
Weibye commented Apr 7, 2023
View reviewed changes
nicoburns
nicoburns reviewed Apr 7, 2023
View reviewed changes
@Weibye Weibye marked this pull request as ready for review April 7, 2023 22:38
@Weibye Weibye mentioned this pull request Apr 8, 2023
Merged
@Weibye Weibye force-pushed the feat/flexbox-feature branch from 5522573 to fe5164c Compare April 8, 2023 07:29
alice-i-cecile
alice-i-cecile approved these changes Apr 8, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good, I really like feature flagging this.

@alice-i-cecile alice-i-cecile added the usability Make the library more comfortable to use label Apr 8, 2023
nicoburns
nicoburns requested changes Apr 9, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but I have a few suggestions. Additionally conflicts with main should be resolved (also clippy lints, but it looks like those have been fixed on main :))

@Weibye Weibye force-pushed the feat/flexbox-feature branch from fe5164c to 3ab7ea7 Compare April 10, 2023 06:34
@Weibye Weibye force-pushed the feat/flexbox-feature branch from 3ab7ea7 to a2e78f7 Compare April 10, 2023 06:39
@Weibye Weibye requested a review from nicoburns April 10, 2023 06:43
@Weibye Weibye mentioned this pull request Apr 10, 2023
Open
@nicoburns nicoburns added the breaking-change A change that breaks our public interface label Apr 10, 2023
@nicoburns
Copy link
Copy Markdown
Collaborator

nicoburns commented Apr 10, 2023

I've pushed a couple of changes:

  • justify_items and justify_self are now gated behind the grid feature only. They do not apply to Flexbox.
  • I renamed a job back to it's original name. This is required to make the github actions pass. @alice-i-cecile I'm not sure if there's some setting we change (perhaps temporarily) to enable the names of github actions jobs to be changed? I think the "required actions" use the names from the main branch, but you can't commit to main without a PR, and you can't get a PR to pass CI with renamed jobs! Not urgent, but would be nice to sort out the names at some point.

@nicoburns nicoburns merged commit 0bccf6c into DioxusLabs:main Apr 10, 2023
@Weibye
Copy link
Copy Markdown
Collaborator Author

Weibye commented Apr 10, 2023

  • justify_items and justify_self are now gated behind the grid feature only. They do not apply to Flexbox.

Right! It's been a while and it's challenging to keep track of all the similar names 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicoburns nicoburns nicoburns approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees

No one assigned

Labels

breaking-change A change that breaks our public interface usability Make the library more comfortable to use

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move flexbox code behind a default feature

3 participants

@Weibye @nicoburns @alice-i-cecile