[MM-10586] Desktop App Window/Tabs Update #1056

devinbinnie · 2019-10-05T15:37:30Z

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
New Server Tabs look and feel.

Issue link
https://mattermost.atlassian.net/browse/MM-10586

…over states and session expired icon

Willyfrog

since its wip, I did a quick review in case it helps you

Willyfrog · 2019-10-07T07:50:36Z

src/browser/components/MainPage.jsx

+    const doubleClickAction = remote.systemPreferences.getUserDefault('AppleActionOnDoubleClick', 'string');
+    const win = remote.getCurrentWindow();
+    if (doubleClickAction === 'Minimize') {
+      win.minimize();
+    } else if (doubleClickAction === 'Maximize' && !win.isMaximized()) {
+      win.maximize();
+    } else if (doubleClickAction === 'Maximize' && win.isMaximized()) {
+      win.unmaximize();
+    }


Maybe you want to have here some sort of strategy pattern, where you don't really care if it's win or mac, but define the behaviours on initialization and here you just apply whatever is configured.

So there's two completely different systems at play here. The Mac one relies on this function and a drag region, whereas the Windows/Linux one is a custom toolbar npm package that handles all this. I'm not really sure how I'd want to 'inject' them separately, since the app doesn't really provides the correct points of insertion, nor do they require the same points.

I think if we were to change the application to be more dependency injection receptive, then it might make sense to modularize these.

Willyfrog · 2019-10-07T07:52:09Z

src/browser/components/MainPage.jsx

+    if (process.platform === 'darwin') {
+      topBarClassName += ' macOS';
+      if (this.state.isDarkMode) {
+        topBarClassName += ' darkMode';


is this for debugging purposes?

No, but I guess that raises the question why I'm using state. I'll switch it to an instance variable.

Actually, I forgot. It should be a state variable, since it can change as the OS changes from light to dark mode.

Willyfrog · 2019-10-07T07:55:04Z

src/browser/components/TabBar.jsx

@@ -71,13 +74,15 @@ export default class TabBar extends React.Component { // need "this"
          title='Add new server'
          draggable={false}
        >
-          <Glyphicon glyph='plus'/>
+          <div className='TabBar-tabSeperator'>
+            {'+'}


are we no longer using fonts for icons?

Not across the board, but the new design uses just a generic +

andrewbrown00 · 2019-10-07T20:04:25Z

@devinbinnie do you know why the Setup Cloud Test Server label isn't available in under Labels? Was trying to add the label so you'd know to spin up a test server whenever you need me to review.

devinbinnie · 2019-10-07T20:08:02Z

@devinbinnie do you know why the Setup Cloud Test Server label isn't available in under Labels? Was trying to add the label so you'd know to spin up a test server whenever you need me to review.

We don't have an automated builder setup for desktop app currently, which I guess is a bit of a problem...

andrewbrown00 · 2019-10-07T21:15:40Z

@devinbinnie thanks for clarifying 😄 Who's the best person to bring this issue up with?

Willyfrog · 2019-10-08T08:30:34Z

@devinbinnie thanks for clarifying 😄 Who's the best person to bring this issue up with?

there is no setup test server but circle does build the app, although it doesn't sign it or package it. there are a couple of steps to get the build:

click on show all checks
on build_and_test click on details
click on store_artifacts-1 which will open circle ci job
click on artifacts tab
select and download the app you will test

at some point it would be great if we posted back with a bot the link to the build, but currently that's not implemented

devinbinnie · 2019-10-11T22:11:24Z

All features complete! Still needs a bit of work here and there, but is mostly ready!

mattermod · 2019-12-17T22:06:36Z

The file .circleci/config.yml is in the blacklist and should not be modified from external contributors, please if you are part of the Mattermost Org submit this PR in the upstream.
/cc @mattermost/core-security

andrewbrown00 · 2019-12-18T22:11:18Z

@esethna answers to your previous comment:

@devinbinnie will handle this
New icons have been provided and are in the latest build
Separate ticket created https://mattermost.atlassian.net/browse/MM-21329
Resolved by #2

esethna · 2019-12-20T01:12:12Z

@devinbinnie is this ready for final review?

devinbinnie · 2019-12-20T05:46:18Z

@devinbinnie is this ready for final review?

Yep!

esethna

Thanks Devin!

esethna · 2019-12-20T18:03:47Z

We can merge this

Willyfrog · 2019-12-23T10:07:13Z

re-running the workflow as it doesn't make any sense to fail there

devinbinnie · 2019-12-23T15:18:48Z

@Willyfrog could you do a quick re-review before we merge to make sure everything is up to standards?

devinbinnie · 2019-12-23T15:19:02Z

/update-branch

Willyfrog

looking mostly good, but I have a few concerns/changes to request

Willyfrog · 2019-12-23T15:48:31Z

package.json

    "mocha": "^5.2.0",
    "mocha-circleci-reporter": "0.0.3",
    "npm-run-all": "^4.1.5",
    "react": "^16.6.3",
    "react-dom": "^16.6.3",
+    "react-smooth-dnd": "github:devinbinnie/react-smooth-dnd#a72095d3fa1fd6ffc3a6b797174d019a07d7b011",


if we are using a fork of a library, shouldn't it be under mattermost and have an associated pull request so we are sure of the changes being introduced?

Yes, though i'm not sure how to go about adding a fork to the MM repo.

You'll have to transfer ownership of it to someone who has admin access to the GitHub organization. I'll message you more details.

Willyfrog · 2019-12-23T15:56:03Z

scripts/fix_catalina_dark_mode_debug.js

@@ -0,0 +1,13 @@
+// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.


why is the file called debug? we should rename it

It's only used for debug mode, I think the name's okay.

Willyfrog · 2019-12-23T16:07:35Z

src/browser/components/MattermostView.jsx

+      case 'mouse-move':
+        this.handleMouseMove(event.args[0]);
+        break;
+      case 'mouse-up':
+        this.handleMouseUp();
+        break;


why do we need to handle movement and release of button?

This is a fix for the tab dragging and dropping, since if you leave the bounds of the DnD area before dropping, it doesn't drop the tab. This does it manually/

src/browser/components/TabBar.jsx

Willyfrog · 2019-12-23T16:52:08Z

src/main.js

+    if (input.key === 'Alt' && input.type === 'keyUp' && altLastPressed) {
+      altLastPressed = false;
+      mainWindow.webContents.send('focus-three-dot-menu');
+      return;
+    }
+
+    // Hack to detect keyPress so that alt+<key> combinations don't default back to the 3-dot menu
+    if (input.key === 'Alt' && input.type === 'keyDown') {
+      altLastPressed = true;
+    } else {
+      altLastPressed = false;
+    }


why is ALT a shorcut? wouldn't that interfere not only with alt+up/down but with anything regarding the OS from the user if he has any shortcut using that? whouldn't it make more sense to use a separate shortcut for that that doesn't use a modifier key as the only one?
non-comprehensive list of things that might break:

alt+f4 on windows (as an example of OS related)

alt+right click for mark as unread (as an example of MM related)

It's a manual override to do something the OS already does, which is open a window context menu when you press ALT. It was part of @esethna's suggestions for accessibility.

I agree it would be better to use a different shortcut, but that's why we're checking for keyup and keydown to make sure it's not being used as a combo.

src/main/Validator.js

Willyfrog · 2019-12-23T16:56:41Z

src/main/Validator.js

+  useSpellChecker: Joi.boolean().default(true),
+  enableHardwareAcceleration: Joi.boolean().default(true),
+  autostart: Joi.boolean().default(true),
+  spellCheckerLocale: Joi.string().regex(/^[a-z]{2}-[A-Z]{2}$/).default('en-US'),


i wouldn't change it in this PR as it has grown big enough, but shouldn't we consider using system's locale as default? we should create a ticket for that

Probably a good idea!

Co-Authored-By: Guillermo Vayá <guivaya@gmail.com>

* [MM-19054] Added new server tab look and feel, still missing proper hover states and session expired icon * [MM-19055] Added window controls and removed border for macOS * [MM-19055] Add dark mode for macOS * [MM-19054] Added session expired icon * Test windows titlebar * Fixed the menu issue and added non-macOS dark mode * Blank commit * Fixed a lint issue * Fixed more lint issues * Fixed more issues * New tray icons * [MM-19603] Drag and drop tabs * Fixed some assets and fixed build output to include missing assets * Fixed a couple small issues * Only show tabs for only 1 server on Mac * Fixed some more tests * Fixed another test * Revert "Fixed another test" This reverts commit 3604029. * Fixed another test * Trial and error! * A bunch of additional fixes * Fixed a lint issue * Fixed restore focus on add server tab causing bad UX * Trial and error on flaky test again * Fixed some bugs based on PR feedback. * blank commit to push tests * Revert "Test windows titlebar" This reverts commit 9cd46b7. * Remove the rest of the old new titlebar and fixes * Added three-dot link * New menu * Rest of new windows menu and other fixes * Fixed lint errors * Added windows 10 style title bar buttons for non mac OS * Lint fixes and enabled the tab bar regardless of number of servers * Missed one * Fixed unicode characters * Commenting out test that should no longer be applicable * Removed Windows 10 style titlebar icons and used material design instead * Fixed a lint issue * Some small UX fixes * blank commit * Fixed an issue where dropping the first tab moves it too far over before snapping into place * Additional style fixes * Another small issue fix * Back to Windows 10 style * Lint fixes * Accessible three dot menu * Lint fixes * Shrinking tabs when window is too small * Gradient between tabs and title bar buttons when window is too small * Add drag to gradient * Replaced icons, drag and drop cursor sticking fix, slight tab change * Lint and some mac fixes * Light theme fix to three dot menu * Hack for tab sticking to cursor on macOS * Fixes for the find utility * Fix for Catalina dark mode * Revert "Fix for Catalina dark mode" This reverts commit 45da05d. * Fixed a couple issues Dean found * More fixes * Three dot hover effect to circle * PR feedback * Test fixes * Test and config fixes * Disable dragging when there are GPO servers * [MM-20757] Fixed dark mode on debug when running macOS Catalina * Allow future config versions to use v2 config if launching this version of the app * Oops * New titlebar icons, blur for titlebar on inactive * Lint fix * Set unfocused opacity to 0.4 * Final FINAL icons * Fixed closing menu not returning focus to the app * Lint fix * Update src/browser/components/TabBar.jsx Co-Authored-By: Guillermo Vayá <guivaya@gmail.com> * Update src/main/Validator.js Co-Authored-By: Guillermo Vayá <guivaya@gmail.com> * Lint fixes * Moved react-smooth-dnd fork to MM org and fixed another merge issue Co-authored-by: mattermod <mattermod@users.noreply.github.com> Co-authored-by: Guillermo Vayá <guivaya@gmail.com>

devinbinnie added 4 commits October 4, 2019 13:17

[MM-19054] Added new server tab look and feel, still missing proper h…

37186b7

…over states and session expired icon

[MM-19055] Added window controls and removed border for macOS

cb76d5d

[MM-19055] Add dark mode for macOS

eb92751

[MM-19054] Added session expired icon

97cbe0b

devinbinnie added Feature null Work In Progress Not yet ready for review 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Oct 5, 2019

devinbinnie added this to the v4.4.0 milestone Oct 5, 2019

devinbinnie requested review from Willyfrog, deanwhillier and andrewbrown00 October 5, 2019 15:37

Willyfrog reviewed Oct 7, 2019

View reviewed changes

devinbinnie added 9 commits October 8, 2019 10:11

Test windows titlebar

9cd46b7

Fixed the menu issue and added non-macOS dark mode

6d620b2

Blank commit

b02c6a9

Fixed a lint issue

496c354

Fixed more lint issues

3caa534

Fixed more issues

96d80ba

New tray icons

39e07b4

[MM-19603] Drag and drop tabs

a22f428

Fixed some assets and fixed build output to include missing assets

d2a8ec0

devinbinnie removed the Work In Progress Not yet ready for review label Oct 11, 2019

devinbinnie added 2 commits October 12, 2019 11:38

Fixed a couple small issues

42c69d6

Only show tabs for only 1 server on Mac

571781f

esethna approved these changes Dec 20, 2019

View reviewed changes

esethna added 4: Reviews Complete All reviewers have approved the pull request and removed 1: PM Review Requires review by a product manager labels Dec 20, 2019

devinbinnie requested a review from Willyfrog December 23, 2019 15:17

Merge branch 'master' into MM-10586

7fb6a45

Willyfrog requested changes Dec 23, 2019

View reviewed changes

devinbinnie and others added 3 commits January 2, 2020 10:57

Merge branch 'master' into MM-10586

0e2e3ab

Update src/browser/components/TabBar.jsx

55e987f

Co-Authored-By: Guillermo Vayá <guivaya@gmail.com>

Update src/main/Validator.js

fc7a8e9

Co-Authored-By: Guillermo Vayá <guivaya@gmail.com>

devinbinnie requested a review from Willyfrog January 2, 2020 16:07

devinbinnie added 2 commits January 2, 2020 11:17

Lint fixes

0f2933e

Moved react-smooth-dnd fork to MM org and fixed another merge issue

7453fe2

Willyfrog approved these changes Jan 3, 2020

View reviewed changes

devinbinnie merged commit 932ddaf into mattermost:master Jan 3, 2020

devinbinnie deleted the MM-10586 branch January 3, 2020 17:00

hunterlester mentioned this pull request Jan 3, 2020

[MM-21469] refactor: e2e test improvements #1142

Closed

3 tasks

amyblais assigned esethna Jan 23, 2020

devinbinnie mentioned this pull request Jan 29, 2020

[MM-20796] Un-reverted the Ctrl+Tab fix and also made sure the tabs go in the right order #1173

Merged

3 tasks

esethna added Docs/Done and removed Docs/Needed labels Feb 12, 2020

mattermod mentioned this pull request Feb 20, 2020

Fix transition when a tab is grabbed so there is no sticking effect #1204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-10586] Desktop App Window/Tabs Update #1056

[MM-10586] Desktop App Window/Tabs Update #1056

devinbinnie commented Oct 5, 2019 •

edited

Loading

Willyfrog left a comment

Willyfrog Oct 7, 2019

devinbinnie Oct 16, 2019

Willyfrog Oct 7, 2019

devinbinnie Oct 7, 2019

devinbinnie Oct 7, 2019

Willyfrog Oct 7, 2019

devinbinnie Oct 7, 2019

andrewbrown00 commented Oct 7, 2019

devinbinnie commented Oct 7, 2019

andrewbrown00 commented Oct 7, 2019

Willyfrog commented Oct 8, 2019 •

edited

Loading

devinbinnie commented Oct 11, 2019

mattermod commented Dec 17, 2019

andrewbrown00 commented Dec 18, 2019

esethna commented Dec 20, 2019

devinbinnie commented Dec 20, 2019

esethna left a comment

esethna commented Dec 20, 2019

Willyfrog commented Dec 23, 2019

devinbinnie commented Dec 23, 2019

devinbinnie commented Dec 23, 2019

Willyfrog left a comment

Willyfrog Dec 23, 2019

devinbinnie Jan 2, 2020

hmhealey Jan 2, 2020

Willyfrog Dec 23, 2019

devinbinnie Jan 2, 2020

Willyfrog Dec 23, 2019

devinbinnie Jan 2, 2020

Willyfrog Dec 23, 2019

devinbinnie Jan 2, 2020

Willyfrog Dec 23, 2019

devinbinnie Jan 2, 2020

		@@ -0,0 +1,13 @@
		// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.

[MM-10586] Desktop App Window/Tabs Update #1056

[MM-10586] Desktop App Window/Tabs Update #1056

Conversation

devinbinnie commented Oct 5, 2019 • edited Loading

Willyfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbrown00 commented Oct 7, 2019

devinbinnie commented Oct 7, 2019

andrewbrown00 commented Oct 7, 2019

Willyfrog commented Oct 8, 2019 • edited Loading

devinbinnie commented Oct 11, 2019

mattermod commented Dec 17, 2019

andrewbrown00 commented Dec 18, 2019

esethna commented Dec 20, 2019

devinbinnie commented Dec 20, 2019

esethna left a comment

Choose a reason for hiding this comment

esethna commented Dec 20, 2019

Willyfrog commented Dec 23, 2019

devinbinnie commented Dec 23, 2019

devinbinnie commented Dec 23, 2019

Willyfrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinbinnie commented Oct 5, 2019 •

edited

Loading

Willyfrog commented Oct 8, 2019 •

edited

Loading