fix(frontend/presenter): clicking on plus should not toggle next line #451
Conversation
AkalUstat
changed the title
|
This looks good to me. Pending final review from Harjot. @AkalUstat In future please use the imperative mood in commit messages (rule 5), meaning "as if giving a command or instruction". So a very small change from @Harjot1Singh might be best to link this in our commit guidelines if not done so already |
| const goNextLine = useCallback( () => { | ||
| const goNextLine = useCallback((e) => { | ||
| // exclude anything other than the main window | ||
| if( e.target.parentNode.className !== 'desktop app' && e.target.parentNode.className !== 'presenter' ) return |
There was a problem hiding this comment.
A better way would be to use the className of the Presenter so that it is resilient to changes in the future (e.g. you can view the controller on iPad, but this approach would not work since it only includes desktop)
There was a problem hiding this comment.
im not sure i understand. Did it this way because sometimes the parent node came out as 'desktop app' or 'presenter' depending on where i clicked
There was a problem hiding this comment.
for example if i clicked the main view area it gave me classname 'desktop app
and at the bottom it gave me 'presenter'
There was a problem hiding this comment.
I meant, programmatically acquire the className from the ref of the presenter. That way, if we change the presenter className for whatever reason, we won't have a regression here because we forgot to update it here too.
There was a problem hiding this comment.
not sure i get it still
| @@ -72,7 +72,9 @@ const NavigatorHotKeys = ( { active, children, mouseTargetRef } ) => { | |||
| } | |||
| }, [ lines, lineId ] ) | |||
|
|
|||
| const goNextLine = useCallback( () => { | |||
| const goNextLine = useCallback((e) => { | |||
There was a problem hiding this comment.
Ey ey, use the linter! Style is wrong
There was a problem hiding this comment.
Also, always prefer being a little more explicit with event over e
yea it works |
excludes the classNames which are not 'desktop app' or 'presenter' fixes shabados#401
|
@bhajneet @Harjot1Singh This is ready for review |
AkalUstat
force-pushed
the
401-dont-advance-plus
branch
from
c942f4c
to
602a90f
Compare
| becomes 'controller-container` instead of presenter. | ||
| In this case, we check the targetClass or its parent node's class | ||
| (which if the target is controller container, will be presenter) */ | ||
| if ( mouseTarget.className !== targetClass |
There was a problem hiding this comment.
We can use De Morgan’s law to make this more readable imo.
if(!(mouseTarget.className === targetClass || mouseTarget.className === parentClass))There was a problem hiding this comment.
Well actually it’s perfectly fine as is
There was a problem hiding this comment.
even better [ targetClass, parentClass ].includes(mouseTarget.className)
puts the two class names in an array and checks if the target className is in the array
| const goNextLine = useCallback( event => { | ||
| const { current: mouseTarget } = mouseTargetRef | ||
|
|
||
| const targetClass = event.target.className |
There was a problem hiding this comment.
Destructure event.target.etc at the top.
| becomes 'controller-container` instead of presenter. | ||
| In this case, we check the targetClass or its parent node's class | ||
| (which if the target is controller container, will be presenter) */ | ||
| if ( ![ targetClass, parentClass ].includes( mouseTarget.className ) ) return |
There was a problem hiding this comment.
I'm not sure if it's just me, but it does look a little strange to have 2 consecutive return statements, consider combining into one?
yea i keep doing brackets for some reason and i use random tags |
and combine both if statements that cancel the event into one
Harjot1Singh
changed the title
Bug introduced during #451, where the checks are too stringent.
excludes the classNames which are not 'desktop app' or 'presenter'
fixes #401