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 URL copy/paster to left sidebar #655

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kanishk98
Copy link
Collaborator

What's this PR do?

Adds a URL container to the left sidebar that displays the URL of the active organization and copies it to the clipboard automatically.
Entering a URL into the container makes the app check if the domain is already saved and then switch to the relevant tab if that's the case.

Any background context you want to provide?

Check out #578 for the same.

Screenshots?

image

Possible improvements
#578 shows a suggestion for a copy button. I couldn't design it as intended (inside the input field), and I think it could make for a nice improvement.

You have tested this PR on:

  • Windows
  • Linux/Ubuntu
  • macOS

Adds a non-functional link button to the left sidebar and associates
tooltip
Associates link button with URL field and preloads URL of currently
active tab
The link field has a default URL filled in corresponding to the
currently active organization. On replacing this with another saved
domain's URL, the app will switch to the relevant organization tab.

Improve field positioning
When user moves from one org to another or to the settings page, the URL
container's visibility is toggled.
@kanishk98
Copy link
Collaborator Author

Hello, just checking in to ask if any changes are required here. :)

@abhigyank
Copy link
Collaborator

@kanishk98 Is the copying limited to just the server name or does it also show the topic or stream part?
While pasting does it go to the the narrow view if we paste a link of a narrow or just focuses on the correct org?

@kanishk98
Copy link
Collaborator Author

kanishk98 commented Mar 31, 2019

Is the copying limited to just the server name or does it also show the topic or stream part?

It's limited to the server URL only.

While pasting does it go to the the narrow view if we paste a link of a narrow or just focuses on the correct org?

There's no web-based stuff involved here. So it'll simply change tabs to another server if the URL of an already saved URL is pasted into the field.

Currently, if a URL belonging to a server that has not been saved is pasted into the field, the app doesn't do anything. I didn't want to add an indication without a discussion. @abhigyank what do you think we should do in that case? Simply clear the field?

@abhigyank
Copy link
Collaborator

It's limited to the server URL only.

Okay, understood. I am not sure if that servers us much purpose here, I mean the chances of someone pasting chat.zulip.org in a toolbar to open the issue would be a very rare case I suppose, unless someone has like 20-30 added organizations (again a very very rare case, max I have seen is about 12).
Typically the use case would be as mentioned in the issue would be to narrow into a view after pasting an url, since it would help people directly opening links from browser or emails (this needs to be done very very carefully as can lead to attacks).

if a URL belonging to a server that has not been saved is pasted into the field, the app doesn't do anything.

This can simple like showing a red exclamation icon or a red boundary - something that websites do to show incorrect passwords dynamically.

@kanishk98
Copy link
Collaborator Author

kanishk98 commented Apr 7, 2019

Typically the use case would be as mentioned in the issue would be to narrow into a view after pasting an url, since it would help people directly opening links from browser or emails (this needs to be done very very carefully as can lead to attacks).

Alright, I understand. I'll be addressing both #470 and this together since they're somewhat similar. Will get to it after my proposal submission, if that's okay.

This can simple like showing a red exclamation icon or a red boundary - something that websites do to show incorrect passwords dynamically.

Easy enough. I'll do this as soon as I can.
Thanks for the review.

@kanishk98
Copy link
Collaborator Author

This can simple like showing a red exclamation icon or a red boundary - something that websites do to show incorrect passwords dynamically.

Done and pushed. Works as shown below.

link

I've already implemented the desired redirection in #716. I think I should wait for a proper review on that before using the same logic here. We can just merge both of them then.

@zulipbot
Copy link
Member

Heads up @kanishk98, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

Base automatically changed from master to main January 22, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants