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

Moving flexbox behind a feature flag #414

Merged
merged 20 commits into from
Apr 10, 2023

Conversation

Weibye
Copy link
Collaborator

@Weibye Weibye commented Apr 7, 2023

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?

src/style/mod.rs Outdated Show resolved Hide resolved
src/prelude.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Weibye Weibye marked this pull request as ready for review April 7, 2023 22:38
@Weibye Weibye mentioned this pull request Apr 8, 2023
@Weibye Weibye force-pushed the feat/flexbox-feature branch from 5522573 to fe5164c Compare April 8, 2023 07:29
Copy link
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
Copy link
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 :))

src/style/mod.rs Outdated Show resolved Hide resolved
src/style/mod.rs Outdated Show resolved Hide resolved
src/style/mod.rs Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
@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
@nicoburns nicoburns added the breaking-change A change that breaks our public interface label Apr 10, 2023
@nicoburns
Copy link
Collaborator

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
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
Labels
breaking-change A change that breaks our public interface usability Make the library more comfortable to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move flexbox code behind a default feature
3 participants