Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Conversion of TopBar to React #997

Merged
merged 14 commits into from
May 20, 2021
Merged

Conversion of TopBar to React #997

merged 14 commits into from
May 20, 2021

Conversation

bbecquet
Copy link
Contributor

@bbecquet bbecquet commented Jan 5, 2021

Description

Introduce a new <TopBar> component:

  • replace the static HTML markup of the top bar
  • improve the Suggest I/O and state management, removing some hacks and sources of bugs
  • encapsulate layout manipulations that were done imperatively outside (focused, filled)
  • remove completely the SearchInput global
  • simplify Menu rendering

The TopBar itself was quite straightforward to move to React. Most of this PR content deals with the second point, the refacto of Suggest, which continues work done in #1079.
The most important part is that the input field itself is now rendered by the Suggest component, but to let the parent control how it renders, I use a render function. This seems a bit complicated at first but is actually better than manipulating a DOM ref all the way. And it makes the life cycles easier to control deterministically.
I also chose to separate more clearly the three different values that the input field can take:

  • the user input
  • the value set when the app panel changes (ex: POI or category name)
  • the value of the suggestion which is currently highlighted with Up/Down keyboard navigation

Previously, confusion between these three values made the different cases hard to follow and debug.

As the DirectionInput also uses Suggest, I had to adapt things in it too.

I tried to do things step-by-step, but could not separate this. And this seemed like the better option to do that when moving the input field in React, as the main reason we used a ref before was to ensure compatibility with the static input.

The downside is this makes a very big PR 😞 Sorry… I propose we do a guided review if you think it's useful.

BTW, I did not push some markup and CSS simplifications that could be also done, because they can wait.

@bbecquet bbecquet marked this pull request as ready for review January 5, 2021 17:44
@bbecquet bbecquet added the WIP label Jan 7, 2021
@bbecquet bbecquet force-pushed the react-top-bar branch 2 times, most recently from afb36e9 to e7506db Compare January 15, 2021 13:47
@bbecquet bbecquet added tech and removed WIP labels Jan 15, 2021
@bbecquet bbecquet force-pushed the react-top-bar branch 2 times, most recently from f0fe4ae to a7a94c3 Compare March 30, 2021 13:29
@bbecquet bbecquet force-pushed the react-top-bar branch 2 times, most recently from c656e0a to b10586e Compare April 27, 2021 09:29
@bbecquet bbecquet marked this pull request as draft April 27, 2021 09:49
@bbecquet bbecquet force-pushed the react-top-bar branch 3 times, most recently from 5aac874 to baf66a4 Compare May 4, 2021 15:20
@bbecquet bbecquet force-pushed the react-top-bar branch 3 times, most recently from ce4b62e to fc8f376 Compare May 10, 2021 15:33
@bbecquet bbecquet changed the title "Direct" conversion of TopBar to React Conversion of TopBar to React May 10, 2021
@bbecquet bbecquet marked this pull request as ready for review May 10, 2021 15:58
@bbecquet bbecquet requested a review from xem May 11, 2021 07:49
Comment on lines 56 to 67
const onSelectSuggestion = (item, query) => {
selectItem(item, { query });
inputRef.current.blur();
};

const onSubmit = async e => {
e.preventDefault();
Telemetry.add(Telemetry.SUGGEST_SUBMIT);
const query = inputRef.current.value;
const results = await fetchSuggests(query, {
withCategories: true,
useFocus: true,
});
onSelectSuggestion(results[0], {
query,
replaceUrl: true,
});
};
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some inconsistencies in the params passed to onSelectSuggestion and selectItem.
According to line 56, the query should not be nested in a object, but the usage of <Suggest onSubmit=...> should be checked too.

As a consequence, an object may be used as the input value:
image

Comment on lines 68 to 82
useEffect(() => {
const handleFocus = () => {
if (inputNode.value === '' || isMobile) {
setIsOpen(true);
fetchItems(inputNode.value);
}
};

const handleBlur = () => {
if (!hasFocus) {
close();
};
} else if (isMobile) {
setIsOpen(true);
}
}, [hasFocus, isMobile]);

const handleInput = e => {
const { value } = e.target;
useEffect(() => {
setHighlighted(null);
if (value !== null) {
fetchItems(value);
setIsOpen(true);
setLastQuery(value);
};
}
}, [fetchItems, value]);
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 useEffect do not handle correctly the case when a empty input is focused. A list of favorites should appear as suggestions and it's no longer the case (probably related to the condition inputNode.value === '' that has been removed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced these 2 effects by a single one, which makes things actually simpler. I hope I haven't overlooked yet another edge-case…

Comment on lines +33 to +34
setUserInputValue(inputRef.current.value + e.key);
inputRef.current.focus();
Copy link
Contributor

@amatissart amatissart May 18, 2021

Choose a reason for hiding this comment

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

It works, but I am not sure to understand why calling setUserInputValue() is necessary.
Isn't the input value supposed to change (and trigger onChange) after the input has been focused ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the point which is driving me mad…
The focus is done and yes this is supposed to work like you say, but the first onChange isn't triggered and the first letter is "lost". Honestly I've not figured exactly why yet, after a long debugging time spent on it…
I think it's related to the management of the user input value in PanelManager, as in one of the previous commits it was managed as local state in TopBar and it worked fine.

Feel free to spend time on it, maybe you'll understand what I wasn't able to, and I would be very interested in another PoV.

But if this is the last remaining hack I guess it's not too bad compared to the previous implementation.

Copy link
Contributor

@amatissart amatissart May 19, 2021

Choose a reason for hiding this comment

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

I am still not sure about what is going on here. Has this behavior ever worked with a "controlled" input? It looks like
React manages a custom cycle of events for keystrokes in this case.

An alternative solution would be to re-dispatch the "keydown" event after the input has been focused. Then, the dependency to setUserInputValue could be removed:

Suggested change
setUserInputValue(inputRef.current.value + e.key);
inputRef.current.focus();
inputRef.current.focus();
inputRef.current.dispatchEvent(new KeyboardEvent(e.type, e));

It may be more consistent with the initial implementation, but I am not sure it is any better.

Copy link
Contributor

@xem xem left a comment

Choose a reason for hiding this comment

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

wow, that's a big, but very clean rewrite!
Works great according to my local tests.
as for the "hack", I agree that it's not too bad if it's just that.

@bbecquet bbecquet merged commit a8e2a4e into Qwant:master May 20, 2021
@bbecquet bbecquet deleted the react-top-bar branch May 20, 2021 08:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants