Skip to content

add spinner and sidebar toggle#21

Open
d-rk wants to merge 2 commits intoduolingo:masterfrom
d-rk:feature/spinner
Open

add spinner and sidebar toggle#21
d-rk wants to merge 2 commits intoduolingo:masterfrom
d-rk:feature/spinner

Conversation

@d-rk
Copy link
Contributor

@d-rk d-rk commented Jan 24, 2022

Hi

I hope you don't mind another PR from me 😰

This PR introduces the following:

  • the sidebar is hidden by default (needed for the spinner)
  • while search for an engine is running a spinner is shown
  • a click on the sidebar first focuses the engine results. a second click collapses the engine results and a third one expands them again
  • links in search results are opened on a new page

<Sidebar
visible={resultGroups.length > 0}
hiddenEngines={localData.hiddenEngines || []}
onToggle={engineId => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the onToggle method here is the same like below in <Results. this needs to be refactored, but I gonna need help for this.

@artnc
Copy link
Member

artnc commented Jan 24, 2022

Could you please post some screenshots? I'm having trouble getting the spinner to appear 🤔

Also I'd prefer to follow standard UI patterns (seen in other search websites like Google) by letting the user decide whether they want to open result links in new tabs (e.g. via middle click or ctrl+click) rather than deciding for them

@d-rk
Copy link
Contributor Author

d-rk commented Jan 25, 2022

Could you please post some screenshots? I'm having trouble getting the spinner to appear 🤔

I tested it again in Firefox and Chrome. Works on my machine :)
spinner

Also I'd prefer to follow standard UI patterns (seen in other search websites like Google) by letting the user decide whether they want to open result links in new tabs (e.g. via middle click or ctrl+click) rather than deciding for them

Thats a valid point. I reverted the changed link open behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants