Ux manage servers #415

jnugh · 2017-01-16T23:01:28Z

Before submitting, please confirm you've

read and understood our Contributing Guidelines
completed Mattermost Contributor Agreement
executed npm run lint:js for proper code formatting

Please provide the following information:

Summary
Adds a button to the Tab Header which allows the user to add a new team without opening the settings page. Use the new Modal in the settings view as well.

Issue link
#400, #401

Test Cases
Add a new server from the tab header and the settings page.

I added multiple specs that test if newly added teams are being persisted and if the modal behaves as expected in general.

I'm not sure if the way I do error handling is a good idea, I just saw that #400 defines an area where the error message should be displayed - I think that it's best to have the message next to the input that is incorrect but I could change it (might be a good idea to discuss).

yuya-oc · 2017-01-17T11:38:41Z

Somehow CircleCI didn't build the PR (probably due to preferences). So I made the artifacts in my fork. https://circleci.com/gh/yuya-oc/desktop/132#artifacts

jnugh · 2017-01-17T11:58:08Z

What's going on with the tests? It worked for me locally...

yuya-oc

valid-url is missing in src/package.json.

jnugh · 2017-01-17T12:03:54Z

That was pretty obvious... 👍

yuya-oc · 2017-01-17T12:38:47Z

Testable artifacts are available. https://circleci.com/gh/mattermost/desktop/839#artifacts

jasonblais · 2017-01-17T13:26:45Z

Yay, this is great @jnugh! Will test it out today! 🎉

jasonblais · 2017-01-19T01:21:18Z

Thanks @jnugh! This is really well done! I've been testing it out yesterday and today.

There are a few notes / pieces of feedback to consider when you have the chance - even though they might seem like a lot, it's just mostly small UX notes. Functionally everything is working really well! Love it :)

1) "Add Server" dialog

Proposals (based on #400, part 3):

VISUAL:

1a) Change title from Add a new Team to Add Server --> a new team sidebar was added to Mattermost Server 3.6, which allows you to display all your teams on the single interface. So you no longer need a separate tab for your teams, only for your servers
1b) I feel Please specify a server name and a valid Mattermost URL is redundant since we give help text below the input boxes - we could consider removing that sentence from the modal, what do you think?
1c) Update https://example.org to https://example.com (it'll be consistent with other similar examples in Mattermost)

FUNCTIONALITY:

Propose that when a user presses
1d) Enter, the "Add" button action is taken
1e) Esc, the "Cancel" button action is taken
1f) Clicking outside of the modal closes the dialog (carries out the "Cancel" button action) --> I'm not sure how hard this would be to implement, so definitely okay making a separate improvement ticket for it
1g) After hitting "Add" on the dialog (when pressing the "+" icon, not on the Settings page), and the new server appears on the right-most tab of the navigation bar, propose automatically activating that tab for the user.

ERROR HANDLING:

1h) Propose that errors aren't displayed until the user hits the "Add" button or hits Enter. This is consistent with other Mattermost dialogs, and is more forgiving to the user (we want to avoid telling the user they made a mistake, unless they actually made one).

When the user hits "Add" and if there's an error that prevents adding the server, then

highlight the input field in red only (no highlights for the title or help text)
show the error message below the input field, but above the help text, similar to when creating a new channel (see below)

1i) Error messages (from #400)

If no name is provided, show text “Name is required."
If no URL is provided, show text “URL is required.”
If URL is provided but is missing http:// or https://, show text “URL should start with http:// or https://.”

2) "Edit Server" dialog

It looks like clicking "Edit" on the Settings page opens the same "Add Server" dialog without pre-populated information.

Propose that

fields are pre-populated with the previously specified server display name and server URL
title is "Edit Server" instead of "Add Server"
pressing "Cancel" undoes changes and closes the modal
pressing "Save" closes the modal and saves the changes

3) "+" icon

3a) Propose removing the border around the "+" icon in server navigation tab bar -- it makes it look a bit prominent (I noticed that my eyes keep getting drawn to it). (Propose we keep the hover effect though)
3b) To follow the "obvious" design principle, propose we add a tooltip when hovering over the "+" icon, similar to Google Chrome (see below) with text New server.

4) Miscellaneous

This is just something that came to mind during testing and would probably be a separate improvement ticket:

If there is only one server tab, then the "+" icon isn't visible since the server navigation bar is hidden automatically. Hence, it's not obvious how to add a new server in that case.

I'm not a huge fan for showing the tab bar when there is only one server, and I think we should continue to hide it. Otherwise, it would just be wasted space on the app window for those who only have one server.

However, we could consider adding a new option in the menu bar such as Sign in to Another Server or Add a New Server, perhaps under File or Window, which opens the "Add Server" dialog when clicked. That way it'll at least be more discoverable.

Other ideas would be welcome.

jnugh · 2017-01-19T21:14:09Z

Alright, most of these things should be pretty easy to change.
Regarding error handling: There is still some stuff to do but I didn't want to focus on the actual behavior before discussing where to place the message (there is this mockup where it goes to the bottom left):

So when I change the style for invalid inputs, that would would be a general change for all inputs in the desktop app (not inside the web view) - To be honest I'm not sure if we highlight invalid input somewhere else yet - if we do these places would change as well. But as this would be consistent with the Mattermost platform I feel like this should actually be the best way to go.

1a-g) 👍
1h-i) See above 👍
2) Nice catch, didn't even notice that I changed the behavior of the server edit function. We could still keep the old edit behavior - might make sense when we add drag/drop reordering...
3) I change the + button into a link - let's see.
4) Would be very easy to add 👍

jasonblais · 2017-01-19T23:02:41Z

Thanks for your response! All sounds good.

1h-i): Ah, you're right. Yes, let's add it at the bottom left instead. User's eyes are drawn towards the "Add" button and hence you notice the error message more easily if it's close to it.

That way you also don't have to move the help text down if there's an error message present.

2): Yeah, I think using the "Edit Server" dialog would be better for a consistent experience. Also, right now the 'edit server' input fields appear at the bottom of the server list, which isn't necessarily clear to users (it looks like you're adding a new server).

This would also cover part 4 of #400 ticket.

3): Let me know if that's tricky to change, happy to discuss it
4): Awesome, can you help add it in that case? We can add it under File > Sign in to Another Server, which just performs the same exact action as the "+" icon.

Propose adding it as the first item on the File drop-down list.

jasonblais · 2017-01-19T23:05:25Z

By the way, @yuya-oc, if you have any feedback about my proposals, please let me know.

Same for you @jnugh, please don't hesitate to suggest other options

yuya-oc

A prop is not a state, so it should not be changed by the component itself. However, probably to fix this is a little difficult in this PR. So please add some comments.

yuya-oc · 2017-01-20T12:20:16Z

src/browser/components/MainPage.jsx

+            this.setState({
+              showNewTeamModal: false
+            });
+            this.props.teams.push(newTeam);


prop is changed.

yuya-oc · 2017-01-20T12:20:48Z

src/browser/components/TeamList.jsx

+            this.setState({
+              showNewTeamModal: false
+            });
+            this.props.teams.push(newTeam);


prop is changed.

yuya-oc · 2017-01-20T12:28:15Z

Almost was done in above discussion 👍
So I added a review in code.

yuya-oc · 2017-01-20T12:32:07Z

src/browser/components/MainPage.jsx

+    var modal;
+    if (this.state.showNewTeamModal) {
+      modal = (
+        <NewTeamModal


Possibly you can use show parameter of ReactBootstrap.Modal. In my memory, it would make better animation. Please see LoginModal in this render().

jasonblais · 2017-01-23T19:19:15Z

Thanks for the continued progress @jnugh!

jasonblais · 2017-01-30T03:25:45Z

Hey @jnugh! Just checking how things are going? :)

…attermost#401

jnugh · 2017-01-30T21:21:12Z

Here comes another bunch of changes :) From what I see, the only thing missing is to change the plus sign... I tried a usual link but that does not match the layout at all. So some more Styling is needed here.

jasonblais · 2017-01-30T23:54:54Z

Awesome! 🎉

Let me know if you need any help with CSS on the plus sign styling, I can ask our UI designer to help take a look too.

yuya-oc · 2017-01-31T13:21:00Z

Please let me confirm. Is the part 4 of #400 intended in this PR? I could not close the modal which is opened by "Edit". And if I click the outside of modal or "Cancel", the server is unexpectedly duplicated.

Repro steps:

Click "Edit" in settings page. ("Add Server" modal opens. This should be "Edit Server".)
Click "Cancel" or outside of the modal.
Click "Add". (should be "Save")
The server is duplicated as a new server.

If the step 2 is skipped, the modal correctly works.

However the key functionality (especially modal) would be done in this PR. So I feel that I can take over the rest though, how do you feel?

jnugh · 2017-01-31T14:28:26Z

That should be fixed now :)
I guess it can be merged if you have no further additions or changes. The Plus button can be changed afterwards...

jasonblais · 2017-01-31T14:48:41Z

Thanks @jnugh! Let me test it out as well, thanks for all your help here!

yuya-oc · 2017-02-01T15:41:01Z

The feature looks working 👍

jasonblais · 2017-02-02T14:45:45Z

Thanks @jnugh! Looking great!

Just minor notes:

Team URL should actually be "Server URL". I missed this on the first review

Can we make the whitespace between
a. top line and Server Display Name
b. bottom line and The URL of ... help text

equal?

Let's use the spacing between the bottom line and The URL of ... help text for both.

Animation when closing the dialog

We have a really nice animation for the "Add Server" dialog when it appears (yay!).

However, the same animation doesn't happen when the dialog closes. It's more abrupt.

Wondering if this is easy to update so the experience is consistent?

I noticed the error handling for http:// isn't correct: it actually accepts a URL with just http:

I couldn't reproduce it with our 3.5 release, so seems it might be caused by this PR.

"+" icon: How likely do you think you'll have time to update the styling of the "+" icon? We're considering to cut the release candidate for 3.6 on Feb 7th.

I could ask our UI guru (asaad) to take a look. It might be quick for him and we' be able to apply it already on this PR.

jnugh · 2017-02-03T17:38:26Z

: I use https://www.npmjs.com/package/valid-url, I might switch to something different - any other ideas?
: Yeah if he has time he can definitely do this a lot faster then I could. Would be awesome!

jasonblais · 2017-02-03T18:28:25Z

4 - What did we use before? @yuya-oc any suggestions?
5 - I'll touch base with him 👍

Thanks @jnugh for your continued help!

jasonblais · 2017-02-03T19:37:25Z

Update: I tested 1) and 2), looks good!

The commit for 3) seems to break the build, so I couldn't test it yet: 1343dce

Also asked Asaad to take a look at 5)

jnugh · 2017-02-03T20:32:34Z

Tests were not waiting for animation - fixed that one.
4) I will use the regex which was used before, drop valid-url.

jasonblais · 2017-02-06T16:32:14Z

@jnugh I tested 3) and 4), works great.

@asaadmahmood will take a look at the styling of the "+" sign once this PR has merged.

@yuya-oc please feel free to merge if this looks good to you

yuya-oc · 2017-02-10T11:19:06Z

Looks good. Thanks @jnugh many times!

yuya-oc added All Platforms null MacOS (looking for testers) Windows (looking for testers) labels Jan 17, 2017

yuya-oc added this to the v3.6.0 milestone Jan 17, 2017

yuya-oc suggested changes Jan 17, 2017

View reviewed changes

yuya-oc suggested changes Jan 20, 2017

View reviewed changes

yuya-oc reviewed Jan 20, 2017

View reviewed changes

jasonblais mentioned this pull request Jan 23, 2017

Ctrl, Shift or Alt + Click on a channel #406

Closed

3 tasks

This was referenced Jan 30, 2017

Desktop UX Improvements: Settings Page #400

Closed

Desktop UX Improvements: Managing servers #401

Closed

jnugh added 8 commits January 30, 2017 20:13

Add an add team button to the TabBar

5f7ab2d

Persist new Teams

aa862b2

📓 Add Changelog entry for new 'add team' button, covers part 1,2 of m…

14c9112

…attermost#401

Use new team modal in settings view as defined in mattermost#400

42c8f4e

Add specs for new team modal

8f3f593

add valid-url to package.json

ec21494

Change labels and texts according to visual feedback

9af3b95

Allow new team modal to be closed using key events

9d11927

Fix failig test

5e55c54

Fix inconsistency when new team modal is beeing used to edit teams

cb84d00

yuya-oc removed the MacOS (looking for testers) label Feb 1, 2017

jasonblais mentioned this pull request Feb 2, 2017

Mattermost Desktop App v3.6 - RC1 is out! #429

Closed

jnugh added 2 commits February 3, 2017 18:41

Team URL should be Server URL

e82f0c3

Remvoe spaceing at the bottom of new team modal

7747236

jnugh force-pushed the ux-manage-servers branch from 1343dce to faa5177 Compare February 3, 2017 20:28

jnugh force-pushed the ux-manage-servers branch from faa5177 to 049e8e0 Compare February 3, 2017 20:47

jnugh added 2 commits February 3, 2017 22:01

Animate NewTeamModal close

63fe562

Use old url validation

9996687

jnugh force-pushed the ux-manage-servers branch from 049e8e0 to 9996687 Compare February 3, 2017 21:01

jasonblais mentioned this pull request Feb 3, 2017

Cannot Back Out of "Choose a Team" Screen #432

Closed

3 tasks

jasonblais removed the Windows (looking for testers) label Feb 6, 2017

yuya-oc approved these changes Feb 10, 2017

View reviewed changes

yuya-oc merged commit c76797e into mattermost:master Feb 10, 2017

jasonblais mentioned this pull request Feb 16, 2017

When name is missing from "Add new server" dialog, only the name field should be highlighted and "Add" button shouldn't be disabled #438

Closed

3 tasks

yuya-oc mentioned this pull request Feb 16, 2017

Open the corresponding tab when a server is clicked in settings page #425

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ux manage servers #415

Ux manage servers #415

jnugh commented Jan 16, 2017

yuya-oc commented Jan 17, 2017

jnugh commented Jan 17, 2017

yuya-oc left a comment

jnugh commented Jan 17, 2017

yuya-oc commented Jan 17, 2017

jasonblais commented Jan 17, 2017

jasonblais commented Jan 19, 2017

jnugh commented Jan 19, 2017

jasonblais commented Jan 19, 2017 •

edited

jasonblais commented Jan 19, 2017

yuya-oc left a comment

yuya-oc Jan 20, 2017

yuya-oc Jan 20, 2017

yuya-oc commented Jan 20, 2017

yuya-oc Jan 20, 2017 •

edited

jasonblais commented Jan 23, 2017

jasonblais commented Jan 30, 2017

jnugh commented Jan 30, 2017

jasonblais commented Jan 30, 2017

yuya-oc commented Jan 31, 2017

jnugh commented Jan 31, 2017

jasonblais commented Jan 31, 2017

yuya-oc commented Feb 1, 2017

jasonblais commented Feb 2, 2017 •

edited

jnugh commented Feb 3, 2017

jasonblais commented Feb 3, 2017

jasonblais commented Feb 3, 2017

jnugh commented Feb 3, 2017

jasonblais commented Feb 6, 2017

yuya-oc commented Feb 10, 2017

Ux manage servers #415

Ux manage servers #415

Conversation

jnugh commented Jan 16, 2017

yuya-oc commented Jan 17, 2017

jnugh commented Jan 17, 2017

yuya-oc left a comment

Choose a reason for hiding this comment

jnugh commented Jan 17, 2017

yuya-oc commented Jan 17, 2017

jasonblais commented Jan 17, 2017

jasonblais commented Jan 19, 2017

1) "Add Server" dialog

2) "Edit Server" dialog

3) "+" icon

4) Miscellaneous

jnugh commented Jan 19, 2017

jasonblais commented Jan 19, 2017 • edited

jasonblais commented Jan 19, 2017

yuya-oc left a comment

Choose a reason for hiding this comment

yuya-oc Jan 20, 2017

Choose a reason for hiding this comment

yuya-oc Jan 20, 2017

Choose a reason for hiding this comment

yuya-oc commented Jan 20, 2017

yuya-oc Jan 20, 2017 • edited

Choose a reason for hiding this comment

jasonblais commented Jan 23, 2017

jasonblais commented Jan 30, 2017

jnugh commented Jan 30, 2017

jasonblais commented Jan 30, 2017

yuya-oc commented Jan 31, 2017

jnugh commented Jan 31, 2017

jasonblais commented Jan 31, 2017

yuya-oc commented Feb 1, 2017

jasonblais commented Feb 2, 2017 • edited

jnugh commented Feb 3, 2017

jasonblais commented Feb 3, 2017

jasonblais commented Feb 3, 2017

jnugh commented Feb 3, 2017

jasonblais commented Feb 6, 2017

yuya-oc commented Feb 10, 2017

jasonblais commented Jan 19, 2017 •

edited

yuya-oc Jan 20, 2017 •

edited

jasonblais commented Feb 2, 2017 •

edited