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

fix: absolute positioning doesn't ignore the padding of the parent element #986

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Aiving
Copy link
Contributor

@Aiving Aiving commented Oct 16, 2024

No description provided.

@Aiving Aiving requested a review from marc2332 as a code owner October 16, 2024 15:36
@marc2332 marc2332 added the enhancement 🔥 New feature or request label Oct 16, 2024
@marc2332 marc2332 added this to the 0.3.0 milestone Oct 24, 2024
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.34%. Comparing base (00ca902) to head (a5c16c9).

Files with missing lines Patch % Lines
crates/torin/src/measure.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #986      +/-   ##
==========================================
- Coverage   74.34%   74.34%   -0.01%     
==========================================
  Files         214      214              
  Lines       24739    24742       +3     
==========================================
+ Hits        18393    18395       +2     
- Misses       6346     6347       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@marc2332 marc2332 left a comment

Choose a reason for hiding this comment

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

What about the root node when measuring it in torin.rs?

@Aiving
Copy link
Contributor Author

Aiving commented Nov 1, 2024

What about the root node when measuring it in torin.rs?

I'm not sure I understand correctly what the question is about. Do you mean absolute positioning of the root element itself or something else?

@marc2332
Copy link
Owner

marc2332 commented Nov 1, 2024

What about the root node when measuring it in torin.rs?

I'm not sure I understand correctly what the question is about. Do you mean absolute positioning of the root element itself or something else?

Torin starts measuring from the root node by calling measure_node, you changed how this method is called when measuring descendants but not when is called upon the root node of the measurement.

By root node I don't specifically refer to the root node of the app but to the root node from where the measuring starts, which could be different is the measure is incremental.

@marc2332
Copy link
Owner

marc2332 commented Nov 1, 2024

Perhaps your way is not a good approach, if the 3 places where this function is called require some special logic maybe its better to move the logic inside or create a simple function to abstract it

@marc2332 marc2332 marked this pull request as draft November 10, 2024 11:35
@marc2332
Copy link
Owner

I don't agree with this, if an element is absolute I still think it should consider the parent's padding. I recently added the global position too, maybe that fits your use case better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🔥 New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants