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

Grid/refactor coordinate handling #301

Merged

Conversation

nicoburns
Copy link
Collaborator

Objective

The handling of grid line coordinates was previously incorrect in a few corner cases (such as specifying -1 span 3). This PR fixes those bugs. It also introduces 2 newtypes GridLine(NonZeroI16) and OriginZeroLine(i16) to represent the coordinate systems

Feedback wanted

What is the best way to expose grid lines to users.

The publicly exposed GridPlacement type is now an alias for GenericGridPlacement<GridLine> (so users who are using the raw types rather than the style helpers must pass a GridLine for the Line variant.

Currently:

  • GridLine is a wrapper around NonZeroI16 (zero is invalid)
  • For ergonomic reasons line() style helper accepts i16, but returns GridPlacement::Auto if zero is passed.

I'm thinking it might be better if:

  • GridLine is a wrapper around i16
  • Documentation is added to line() to indicate that zero is invalid (and point to MDN docs on grid lines)
  • line() returns GridPlacement::Line(GridLine(0)) if zero is passed.
  • GridPlacement::Line(GridLine(0)) gets converted to GridPlacement::Auto internally.

The reason I'm thinking this is that I reckon we'll probably want some kind of debug tooling to introspect styles at some point, and for that we'll need to represent exactly what the user specified (and then indicate it's invalid).

Other alternatives:

  • Make the user manually create a NonZeroI16 (but this is incredibly verbose)
  • Make line() panic if passed zero (but I think we ought to avoid panics wherever possible).

@alice-i-cecile alice-i-cecile added the bug Something isn't working label Dec 28, 2022
@alice-i-cecile
Copy link
Collaborator

I agree with your assessment of the options. Panicking is bad, and NonZero is a giant pain for anyone writing this out by hand (or serializing it).

@nicoburns
Copy link
Collaborator Author

I agree with your assessment of the options.

Excellent. This has now been implemented :)

@nicoburns nicoburns marked this pull request as ready for review December 28, 2022 04:19
@nicoburns nicoburns changed the title WIP: Grid/refactor coordinate handling Grid/refactor coordinate handling Dec 29, 2022
@nicoburns nicoburns force-pushed the grid/refactor-coordinate-handling branch 2 times, most recently from 9eb1b5c to a57c752 Compare December 30, 2022 10:53
Copy link
Collaborator

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

This improves "correct" usage, so I really like these changes.

src/style/grid.rs Outdated Show resolved Hide resolved
src/style/grid.rs Outdated Show resolved Hide resolved
@@ -28,12 +28,12 @@ pub(crate) fn compute_grid_size_estimate<'a>(
// Compute *track* count estimates for each axis from:
// - The explicit track counts
// - The origin-zero coordinate min and max grid line variables
let negative_implicit_inline_tracks = col_min.unsigned_abs();
let negative_implicit_inline_tracks = col_min.0.unsigned_abs();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This diff is an indication that we should be using Deref / DerefMut impls on these newtypes IMO.

Copy link
Collaborator Author

@nicoburns nicoburns Jan 2, 2023

Choose a reason for hiding this comment

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

I'm a bit wary of implementing Deref/DerefMut for the coordinate newtypes as a large part of the purpose of the types is to make sure that they are not used in the wrong context and I feel like this would weaken that protection. I've taken an alternative approach of:

  • Adding methods to OriginZeroLine. They still use .0 internally, but these .0's are now constrained to the impls for that type (i.e they're all self.0).
  • Just using OriginZeroLine directly in more places (avoiding the need to access the inner value entirely).

Does that seem acceptable to you?

I've also uncovered a couple of bugs in the process of this, which all seemed to affect grid items that are auto-placed into negative grid tracks. These now have gentests. At some point I would like to go through and convert all the placement unit tests to gentests, but that's definitely for a separate PR.

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.

Generally in favor of this! Two requests:

  1. Deref / DerefMut on the newtypes: there's a ton of .0 churn in this diff that makes it unclear.
  2. Change log.

@nicoburns nicoburns force-pushed the grid/refactor-coordinate-handling branch from d29da94 to 7408fdc Compare January 2, 2023 11:55
@nicoburns nicoburns force-pushed the grid/refactor-coordinate-handling branch from 47279cd to 4a64cd9 Compare January 2, 2023 12:20
@nicoburns nicoburns force-pushed the grid/refactor-coordinate-handling branch from b8dc8ea to 77728d3 Compare January 2, 2023 13:56
@nicoburns
Copy link
Collaborator Author

Release notes updated. User-facing changes in this PR are pretty minor, but there is one small one so I've included it.

@alice-i-cecile alice-i-cecile merged commit 859cc92 into DioxusLabs:main Jan 2, 2023
@nicoburns nicoburns deleted the grid/refactor-coordinate-handling branch January 2, 2023 18:37
@nicoburns nicoburns mentioned this pull request Jan 4, 2023
87 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants