Skip to content
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

Add help tour for ConnectForm (connecting to server and logging in) #199

Merged
merged 9 commits into from
Sep 8, 2021

Conversation

christophfriedrich
Copy link
Collaborator

@christophfriedrich christophfriedrich commented Sep 3, 2021

This PR adds a help button in the upper right corner of the "login bubble":

grafik

And the corresponding tour steps of course, see code or try for yourself :)

This way the icons of the Next and Previous buttons are always on the outer sides, which is more natural in my opinion.
"Klein aber fein"
Usually in the application text is black on white with blue links. Here we have white text on blue background, so in turn the links need to be black.
I think it looks better when the little arrow does NOT have a border too.
Specifying the targets directly is a lot more versatile. With the previous method of always adding a class to the element that is named after a convention, this tour wouldn't be possible like this (because it's impossible to add custom classes to elements that are created by the Tabs component, and because it wouldn't be possible to dynamically select the input field that is currently visible).
@m-mohr m-mohr added this to the v0.9.0 milestone Sep 3, 2021
@m-mohr
Copy link
Member

m-mohr commented Sep 7, 2021

@christophfriedrich I should merge this tomorrow afternoon at the latest, can you finalize the PR until then? The 0.9.0 release should include this for SRR2 and thus I have to release Thu morning.

@christophfriedrich
Copy link
Collaborator Author

Yes I can finish it by then 👌

christophfriedrich and others added 2 commits September 7, 2021 18:01
Reword "URL" as "server address", remove words like "icon", "browser" etc.
Some texts more or less entirely rewritten.
Also open link in new tab.

Co-authored-by: Matthias Mohr <[email protected]>
No URL, history or switching, instead a help message might be useful for the case that the configured server is not reachable.
Also mention console key for Firefox in the alert ;)
@m-mohr
Copy link
Member

m-mohr commented Sep 7, 2021

Is this ready to be merged? @christophfriedrich

@christophfriedrich
Copy link
Collaborator Author

The help text redesign is still missing, just a few more minutes 😉

@m-mohr
Copy link
Member

m-mohr commented Sep 7, 2021

Take your time, I'll only merge tomorrow.

Help "button" is now blue like the "Switch server" label etc., links get underlined when hovered.
When a server doesn't have basic auth or no login field is currently in view, the corresponding tour bubbles won't be part of the tour.
Also give the help button some margin so that it still looks nice even when the server name is long.
@christophfriedrich
Copy link
Collaborator Author

christophfriedrich commented Sep 7, 2021

All conversations are now resolved, and I even managed to make tour steps for elements that aren't always there "on demand". Which was important, because otherwise when a server doesn't support ALL things that we would have explanations for, the tour would crash because the bubble's target isn't there.

In the same fashion, I guess we could also add the more fine-grained things that were requested in #199 (review).

The only issue I still encountered and can't really think of a way to fix: When a user is on the e.g. "Internal" tab, starts the tour, and then switches over to "No credentials", the tour crashes because the step for the input fields (login-credentials) was already loaded, but the target has since been removed from the DOM...

@m-mohr
Copy link
Member

m-mohr commented Sep 8, 2021

What does crash mean? Does it just stop the tour? And then a user would need to restart?

@m-mohr m-mohr merged commit 6e22561 into master Sep 8, 2021
@m-mohr m-mohr deleted the helptour branch September 8, 2021 09:58
@m-mohr
Copy link
Member

m-mohr commented Sep 8, 2021

Yeah, so it stops. That's fine for now, but certainly something we should try to improve later. Merged, thanks!

@christophfriedrich
Copy link
Collaborator Author

Yes, it stops, just disappears. And I wasn't able to restart either. But I think it's an edge case, so yeah we can improve that later.

@m-mohr
Copy link
Member

m-mohr commented Sep 8, 2021

I was able to restart, but we should have an eye on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants