Skip to content

Use the whole list item as the click target #3963

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.

Sign up for GitHub

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

Merged
julienw merged 1 commit into from
Mar 30, 2022
Merged

Use the whole list item as the click target #3963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 56 additions & 37 deletions src/components/shared/FilterNavigatorBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,53 @@ import classNames from 'classnames';
import { CSSTransition, TransitionGroup } from 'react-transition-group';
import './FilterNavigatorBar.css';

type FilterNavigatorBarButtonProps = {|
onClick: (number) => mixed,
index: number,
children: React.Node,
type FilterNavigatorBarListItemProps = {|
+onClick?: null | ((number) => mixed),
+index: number,
+isFirstItem: boolean,
+isLastItem: boolean,
+isSelectedItem: boolean,
+title?: string,
+additionalClassName?: string,
+children: React.Node,
|};

class FilterNavigatorBarButton extends React.PureComponent<FilterNavigatorBarButtonProps> {
class FilterNavigatorBarListItem extends React.PureComponent<FilterNavigatorBarListItemProps> {
_onClick = () => {
const { index, onClick } = this.props;
onClick(index);
if (onClick) {
onClick(index);
}
};

render() {
const {
isFirstItem,
isLastItem,
isSelectedItem,
children,
additionalClassName,
onClick,
title,
} = this.props;
return (
<button
type="button"
className="filterNavigatorBarItemContent"
onClick={this._onClick}
<li
className={classNames('filterNavigatorBarItem', additionalClassName, {
filterNavigatorBarRootItem: isFirstItem,
filterNavigatorBarSelectedItem: isSelectedItem,
filterNavigatorBarLeafItem: isLastItem,
})}
title={title}
onClick={onClick ? this._onClick : null}
>
{this.props.children}
</button>
{onClick ? (
<button type="button" className="filterNavigatorBarItemContent">
{children}
</button>
) : (
<span className="filterNavigatorBarItemContent">{children}</span>
)}
</li>
);
}
}
Expand Down Expand Up @@ -57,22 +83,17 @@ export class FilterNavigatorBar extends React.PureComponent<Props> {
classNames="filterNavigatorBarTransition"
timeout={250}
>
<li
className={classNames('filterNavigatorBarItem', {
filterNavigatorBarRootItem: i === 0,
filterNavigatorBarBeforeSelectedItem: i === selectedItem - 1,

@julienw julienw Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this class as it was clearly not used.

@canova canova Mar 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing!

filterNavigatorBarSelectedItem: i === selectedItem,

@julienw julienw Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered removing this idea of selectedItem as currently it's always the last item. But finally I decided to keep it just in case we were using it in the future.

filterNavigatorBarLeafItem: i === items.length - 1,
})}
<FilterNavigatorBarListItem
index={i}
onClick={
i === items.length - 1 && !uncommittedItem ? null : onPop
}
isFirstItem={i === 0}
isLastItem={i === items.length - 1}
isSelectedItem={i === selectedItem}
>
{i === items.length - 1 && !uncommittedItem ? (
<span className="filterNavigatorBarItemContent">{item}</span>
) : (
<FilterNavigatorBarButton index={i} onClick={onPop}>
{item}
</FilterNavigatorBarButton>
)}
</li>
{item}
</FilterNavigatorBarListItem>
</CSSTransition>
))}
{uncommittedItem ? (
Expand All @@ -81,18 +102,16 @@ export class FilterNavigatorBar extends React.PureComponent<Props> {
classNames="filterNavigatorBarUncommittedTransition"
timeout={0}
>
<li
className={classNames(
'filterNavigatorBarItem',
'filterNavigatorBarLeafItem',
'filterNavigatorBarUncommittedItem'
)}

@canova canova Mar 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to move that inside the FilterNavigatorBarListItem component!

<FilterNavigatorBarListItem
index={items.length}
isFirstItem={false}
isLastItem={true}
isSelectedItem={false}
additionalClassName="filterNavigatorBarUncommittedItem"
title={uncommittedItem}

@canova canova Mar 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, interesting that we are putting a title only for uncommited items, and this is exactly the same text we put inside the button as well 🤔 I feel like this is redundant (but not a blocker for this PR since this was a preexisting issue).

@julienw julienw Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wondered about that too...

I believe we don't do it for other items directly currently because they're <Localized> React nodes, so they're not pure strings that we can reuse. I believe we had titles for everything in the past, and it could be useful in the transform navigator (not so much for the range navigator). Indeed in the transform navigator the text can be cut with an ellipsis sometimes.
If we want to do them in a localizable way, then this would need some more work, I believe that's why it hasn't been done earlier.

>
<span className="filterNavigatorBarItemContent">
{uncommittedItem}
</span>
</li>
{uncommittedItem}
</FilterNavigatorBarListItem>
</CSSTransition>
) : null}
</TransitionGroup>
Expand Down
6 changes: 3 additions & 3 deletions src/test/components/__snapshots__/FilterNavigatorBar.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ exports[`app/ProfileFilterNavigator renders ProfileFilterNavigator properly 2`]
class="filterNavigatorBar profileFilterNavigator"
>
<li
class="filterNavigatorBarItem filterNavigatorBarRootItem filterNavigatorBarBeforeSelectedItem"
class="filterNavigatorBarItem filterNavigatorBarRootItem"
>
<button
class="filterNavigatorBarItemContent"
Expand All @@ -47,7 +47,7 @@ exports[`app/ProfileFilterNavigator renders ProfileFilterNavigator properly 3`]
class="filterNavigatorBar profileFilterNavigator"
>
<li
class="filterNavigatorBarItem filterNavigatorBarRootItem filterNavigatorBarBeforeSelectedItem"
class="filterNavigatorBarItem filterNavigatorBarRootItem"
>
<button
class="filterNavigatorBarItemContent"
Expand All @@ -67,7 +67,7 @@ exports[`app/ProfileFilterNavigator renders ProfileFilterNavigator properly 3`]
</button>
</li>
<li
class="filterNavigatorBarItem filterNavigatorBarLeafItem filterNavigatorBarUncommittedItem filterNavigatorBarUncommittedTransition-enter filterNavigatorBarUncommittedTransition-enter-active"
class="filterNavigatorBarItem filterNavigatorBarUncommittedItem filterNavigatorBarLeafItem filterNavigatorBarUncommittedTransition-enter filterNavigatorBarUncommittedTransition-enter-active"
title="100μs"
>
<span
Expand Down
2 changes: 1 addition & 1 deletion src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3617,7 +3617,7 @@ exports[`calltree/ProfileCallTreeView TransformNavigator renders with multiple t
</button>
</li>
<li
class="filterNavigatorBarItem filterNavigatorBarBeforeSelectedItem"
class="filterNavigatorBarItem"
>
<button
class="filterNavigatorBarItemContent"
Expand Down