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

Implement CSS Grid #205

Merged
merged 208 commits into from
Dec 22, 2022
Merged

Implement CSS Grid #205

merged 208 commits into from
Dec 22, 2022

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Jul 24, 2022

Objective

Fixes #204

Feedback wanted

Nothing in particular just yet. This PR is still very incomplete. Just putting it up early to track progress.

@nicoburns nicoburns closed this Jul 24, 2022
@nicoburns nicoburns reopened this Jul 24, 2022
@alice-i-cecile alice-i-cecile added enhancement New feature or request controversial This work requires a heightened standard of review due to implementation or design complexity labels Jul 24, 2022
@TimJentzsch
Copy link
Collaborator

FYI, if your work is still in progress it's good practice to mark the PR as draft: GitHub Docs

This prevents merging of the PR before it's ready.

You should be able to mark it as draft retroactively in the sidebar on the right, if IIRC.

docs/style-properties.md Outdated Show resolved Hide resolved
src/style.rs Outdated Show resolved Hide resolved
src/style.rs Outdated Show resolved Hide resolved
@nicoburns nicoburns force-pushed the css-grid branch 2 times, most recently from 0bc2100 to feeee35 Compare August 6, 2022 14:36
@nicoburns nicoburns mentioned this pull request Aug 16, 2022
87 tasks
@alice-i-cecile
Copy link
Collaborator

As a heads up @nicoburns, I'd like to merge this relatively aggressively, then iterating on it in follow-up PRs before the next major version. Great work so far!

It's a much easier workflow for collaboration than trying to get it perfect in the very first PR.

@nicoburns
Copy link
Collaborator Author

@alice-i-cecile Good to know! I can break this up into smaller PRs if that would be helpful? And maybe add feature flags for flexbox and grid (flexbox defaulting to on, grid to off for now)?

However this PR builds on top of #202 (as it already much of the work of nicely factoring out the existing flexbox implementation into it's own module). I'm not sure what the status on that PR is, but currently this one would be blocked on that one being merged.

@nicoburns nicoburns changed the base branch from jk/abstract-over-storage to main October 13, 2022 10:55
@nicoburns nicoburns force-pushed the css-grid branch 2 times, most recently from cab1677 to ae69abb Compare October 13, 2022 11:58
@nicoburns nicoburns force-pushed the css-grid branch 3 times, most recently from 0107203 to 610f013 Compare November 23, 2022 13:42
@alice-i-cecile alice-i-cecile added this to the 0.3 milestone Nov 23, 2022
@nicoburns nicoburns force-pushed the css-grid branch 7 times, most recently from ccc420b to 9b0612d Compare November 30, 2022 13:56
@nicoburns
Copy link
Collaborator Author

@alice-i-cecile

I believe I have addressed all of your concerns/requests (in particular I've filled in the release notes). I have also:

  • Implemented absolute and hidden positioning
  • Fixed support for children with margins (including absolutely positioned children).
  • Added a whole bunch of tests and fixes. We're up to 100 gentests for grid now (compared to 277 for Flexbox). Still a lot more testing to be done, but I think this is a good start.
  • Updated CI to run tests both with and without the grid feature flag. Unfortunately the only way I could get this to work was remove it from the "taffy" section in Cargo.toml, which means that my editor doesn't seem to pick up grid related files by default, cargo test doesn't include grid tests by default, and clippy doesn't lint grid files by default (you have to specify --features experimental_grid). Not sure if there is anything we can do about this. For now I've been adding it back it temporarily but not committing it. Perhaps we should make it a default features? Not sure.

As always, progress is being tracked over at #204. My suggestion is that we try to get this PR merged ASAP, but that we keep the issue open and continue to use it to track overall progress on Grid. I imagine the release notes might need some work, but I would like to suggest we start a new release issue/PR and don't block this PR on that.

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.

Awesome write-up! That was my last blocking request: I think that merging and iterating is the way to go now.

@alice-i-cecile alice-i-cecile merged commit 6fd2c6b into DioxusLabs:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controversial This work requires a heightened standard of review due to implementation or design complexity enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support CSS Grid
4 participants