Fix horizontal scrollbar #161
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shpaass
reviewed
Jun 17, 2024
DaleStan
previously requested changes
Jun 18, 2024
veger
force-pushed
the
fix-horizontal-scrollbar
branch
from
541564b
to
d15fd2a
Compare
veger
force-pushed
the
fix-horizontal-scrollbar
branch
2 times, most recently
from
c1ce681
to
79df4f8
Compare
Now they actually reflect what they are about
* Get rid of misleading height property (a visible area height is needed instead) * MeasureContent only uses width, so replace Rect param * Rename Scrollbar Rects to make their purpose more clear * Document code blocks to clarify a little better what happens
rect.Heigth does not need the correction for the ScrollbarSize when not building. It does not have the correction when it is building. So with this fix both the building and not building states use the same rect.Height.
Possible since last fix
The height of the horizontal scrollbar is already taken care of by Scrollable. So it sohuld not be part of the height calculation. The calculation got messed up, when ScrollbarSize != 1 indicating the old calculations were wrong already.
It does not 'de-highlight' and it was only active for the Summary view (making it more consistent with the rest of YAFC)
veger
force-pushed
the
fix-horizontal-scrollbar
branch
from
79df4f8
to
4a8c135
Compare
|
I'm still seeing a .5f vertical offset in the hit testing for both scrollbars. Is that just a me thing? Good work on all the documentation and renaming |
|
I cannot reproduce the .5f offset. I tried with both a 1f and a 4f scrollbar sizes, and with either size clicks that are just outside of the 'scroller' are not registered and you cannot drag it. I see only two (visual) glitches:
|
DaleStan
dismissed
their stale review
Veger can't reproduce issue I'm seeing, and it's minor.
DaleStan
approved these changes
Jun 19, 2024
This was referenced Jun 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I figured out that clicking the horizontal scrollbar was not possible because the 'click test' area did not match the actual drawn rectangle. This got fixed by d9f8151.
In order to figure this out, I did some major documenting/cleanup to get an understanding of what Scrollable class was (intending) to do.
I hope that all new variable/field names make more sense now, if not let me know and we can figure out how to further improve them.
While I was at it I found some other 'oddities' that I fixed/improved:
I assume it would be easiest to review ecah commit separately, as the cleanup shuffled/renames quite come code. Which is much more apparent when reviewing the separate commits.