panningDirection parameter added to InteractiveViewer #109014
Conversation
flutter-dashboard
bot
added
the
framework
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
NazarenoCavazzon
changed the title
|
@NazarenoCavazzon Thanks for the PR! Overall I think this parameter makes sense, I agree that alignPanAxis doesn't quite do what you want. It looks like you ran a formatter on these files, though. Would you be able to undo the formatter changes so that this is easier to review? |
|
Yep, didn't saw it hahahaha, I'll undo it |
|
I have also a question, do I need to update the flutter version or make a document like this one explaining the changes and the tests I added? |
|
@justinmc I reverted the formatting, let me know if you need something |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Thanks for undoing the formatting, much easier to read now! A few small comments below.
If someone specified panningDirection, then there is no reason to specify alignPanAxis too, right? Maybe add an assert to make sure that both aren't set at the same time?
Or, maybe the two parameters should be combined? What if there was just one parameter, an enum (maybe called panAxis?) with values horizontal, vertical, both, and aligned.
To answer your question about writing a design doc, I think it's probably not necessary since this feature is relatively small and not super controversial. It's always welcome if you want to write one though :)
|
I like more the enum approach, it makes more sense, thanks for the review, I'll make the changes ASAP |
|
@justinmc I made the alignPanAxis and Enum. |
|
Everything ready 2 review |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I really like the enum approach, thanks for doing that!
The main thing is just my comment about using a new name and deprecating alignPanAxis.
- Modified the new parameter to be called panAxis - Restored alignPanAxis and added the deprecated annotation - Default value of panAxis changed from both to free - Modified some comments - Added 3 more test for panAxis.vertical and 1 for panAxis.free
…viewer-panning-direction
|
Ready 2 review |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM with nits 👍
Can you search through the code and make sure alignPanAxis isn't referenced anywhere in comments or anything? I think I found one case below.
| this.scaleFactor = 200.0, | ||
| this.transformationController, | ||
| required Widget this.child, | ||
| }) : assert(alignPanAxis != null), |
There was a problem hiding this comment.
Can you keep this assert here for now? Once alignPanAxis is removed then this assert should be removed.
There was a problem hiding this comment.
Added it again
| ) | ||
| final bool alignPanAxis; | ||
|
|
||
| /// When set to [PanAxis.aligned] panning is only allowed in the horizontal |
There was a problem hiding this comment.
Add a comma: "When set to [PanAxis.aligned],"
| alignedTranslation = _alignAxis(translation, Axis.vertical); | ||
| break; | ||
| case PanAxis.aligned: | ||
| alignedTranslation = _alignAxis(translation, _panAxis!); |
There was a problem hiding this comment.
Can you update the comment at the definition of _panAxis? It still references alignPanAxis.
Also it's kind of confusing that there is both panAxis and _panAxis, but they mean different things and have different types... Maybe if we just change _panAxis to _panningAxis or _currentAxis or something? Up to you.
…viewer-panning-direction
…viewer-panning-direction
…viewer-panning-direction
justinmc
added
the
autosubmit
|
Thanks for your persistence on this PR and for fixing those last few comments! This will be merged when the build is green. |
|
Thanks for reviewing it!. After this change I think it would be a great idea to update this widget of the week https://youtu.be/zrn7V3bMJvg hahahah. |
3 participants
It adds a new Axis? parameter to the Interactive Viewer called panningDirection that let's you lock the panning to a certain axis.
Fixes #108935
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.