You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Image preview modal pulled a screen a little when user is zooming the image.
Aparently that's the problem of all the modals - they don't block body.
Fixed here.
- continue to pan when cursor is out of the image
- don't close modal if user released cursor out of the image
- user-select: none to always prevent from selection
- class component is just more readable
Could you elaborate more about what issues it fixed?
The problem I have with introducing class components is that React going forward is moving further and further from this, and the mindshare is already entirely shifted into hooks being the canonical way to develop with React. As a result this means all main libraries are hooks only, including state management approaches. I would not want to have to be maintaining 2 methods of developing components, so it would have to be a definitive reason to consciously move towards.
As for the more readable, that is a subjective take, as I actually see the opposite. What I do agree with is that there are far too many cases of components which happened to use hooks incorrectly which clouds things quite a bit. That is why I framed it more of the mindset that we need to tighten the belt on component best practices, including (must include this) guidance on state management from top to bottom so we don't continue to perpetuate what I consider useEffect / useState hell. No one can reason about that type of state mismanagement, but I don't see that as a React hooks problem, more just a miss entirely on needed refactors when components graduate from a couple pieces of state with a simple interaction, to something greater.
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.
UX fixes:
user-select: noneto always prevent from selectionImage Preview is rewritten to be a class component, it's more readable and fixes some issues.
All modals in DM now fixes the window to be non-scrollable — that's usual behaviour for modals and we just missed this.
PR fulfills these requirements
Change has impacts in these area(s)
What feature flags were used to cover this change?
fflag_feat_front_leap_1424_grid_preview_shortDoes this PR introduce a breaking change?
All modals in DM are affected, but the fix is very expected.
What level of testing was included in the change?