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.
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
[RNMobile] Inner Block Navigate down through hierarchy #17547
[RNMobile] Inner Block Navigate down through hierarchy #17547
Changes from 70 commits
6da1bf4
6387115
e010328
09359f6
a424c63
2e7204b
5dce162
76340c0
3295957
a28b17d
f00ad33
5326d44
c70697c
eb7bed0
2fdedb7
d602fff
497dcbc
ca22502
dd06cba
ec97815
3bf893b
8b02aae
4c1e1b9
e5976c0
b54fa56
a6dcf70
63b163f
d7434b0
ed9a435
aafa452
dffabf9
4576c6a
90ae894
d8d4e1d
677aade
168c3a2
d8b05e2
e8464a0
7a500e1
132fa7a
f94c828
845f920
35d8c8b
7eba06d
48a62f4
65b8400
bd2a9fc
c072555
f06f6af
f2be82d
e47908d
e8ae8b2
84d8dad
014e3a0
fdb2fae
3b94490
941baef
df0f5a4
8ab7ebe
3ebc50c
a035030
cd5b608
bd5a1f8
3219ad0
e3b540a
f47e6dc
ca7e08f
8de07ef
93ae250
662008e
6414b89
524e0ca
7cf3950
5a456c9
061257a
7fb084a
583650b
4574daf
22d5904
75482c5
0814bb8
7cb1ddb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about create const with
getBlockParents( selectedBlockClientId )
and use it instead of calling it 2 times?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading this comment, it did look a bit related to the
firstToSelect
search, so I think we could have the equivalent (please double check my logic here 😉):isDescendantSelected
: if the selection is one of the descendants, then this must be a parent of the selection, which also means it will be the lowest common ancestor. SoisDescendantSelected = commonAncestor === clientId
isDescendantOfParentSelected
: similar to the previous one, if the parent of this block is an ancestor of the selection, but the selection is not one of this block's descendants, then the lowest common ancestor if this block's parent:isDescendantOfParentSelected = commonAncestor === parentId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koke I temporary go with @dratwas suggest.
However I agree with this one
isDescendantSelected = commonAncestor === clientId
but not sure about this
isDescendantOfParentSelected = commonAncestor === parentId
Shouldn't we check the common ancestor with parent of selected block ?
Then it will look like this:
const commonAncestorParent = getLowestCommonAncestorWithSelectedBlock( parentId );
const isDescendantOfParentSelected = commonAncestorParent === parentId;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think that would change the behavior a bit. The
commonAncestor == parentId
would not include the case whereisDescendantSelected == true
. It would be more accurate to sayisDescendantOfSiblingSelected
, which might not be what you had in mind when doing the calculations. I think we can combine both:But at this stage, this feels more like nitpicking, so I'm happy with either approach, we shouldn't block on this one to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those conditions looks quite hard to understand! Could we try to make them more readable? I haven't spent that much time trying to understand it yet so I might have omitted some edge cases, but from the few cases I can think of, the following conditions would be more readable imo:
I'm not sure how
isDescendantOfParentSelected
could play a role here, is there a case where a component is highlighted when a "sibling" is selected?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand,
isDimmed
should only be true for blocks that are outside the selection and its descendants, right? There was a suggestion to do a different think, but as far as I understand we didn't decide to go that way. It's not clear to me if we want to dim other blocks when a top level one is selected (even if it has children), or just when we start going 1+ level deep into nested blocks.There are some redundant checks:
isDescendantSelected
⇒isDescendantOfParentSelected
. SoisDescendantSelected || isDescendantOfParentSelected == isDescendantSelected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my version it's the case when you select first child of root block. Without this check the rest of the components stays highlighted.
Next thing is that I need to dim only the bottom most descendant in each branch which should be dimmed to avoid multiplication of opacity applied to block in hierarchy. In other words if I apply opacity to block parent which fits criteria to be dimmed and them apply opacity for the block itself the block will have opacity 0.4 instead of desire 0.2. This opacity increased depending of nesting level. I hope that make sense
There is also edge case that do not dim when RootList block is selected as well.
I gave a shot for what you proposed and it does not works as expected but I will try to refactor original checks to increase clarity and write some comments in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koke I have noticed your comment now so I want to refer to what you wrote as well
That's true according to same branch.
The case for check 'isDescendantOfParentSelected' according to below image is when we have
1-4
selected (red) andisTouchable
should be set to true for1-2
,1-5
. Also1-4-5
,1-4-3
should be touchable(green). The blue one circle represents block that should be dimmed.I agree that we can simplify this:
const isTouchable = isSelected || isDescendantSelected || isDescendantOfParentSelected || isParentSelected || parentId === '';
to this:
const isTouchable = isSelected || isDescendantOfParentSelected || isParentSelected || parentId === '';
1-5
should haveisDescendantOfParentSelected
true andisDescendantSelected
false because1-4
is not
descendant of1-5
butis
descendant of1-5
parentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, I knew one of them was redundant but I got my logic backwards 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure about
isDimmed
and try to check if we can simplify