App Header React migration #4245

ranbena · 2019-10-15T06:40:53Z

Refactor

TODO

React migration
Responsive layout
Mobile layout - now with fixed position and better UX
All tests pass

Changes

No faves

Favorite lazy loading update

Create button options (added "New" text)

Manually tested

Firefox, Safari, Chrome on Mac
Different viewport widths
Verified redundancy of deleted files
Verified permission defined rendering
Verified update available state

ranbena · 2019-10-15T14:44:03Z

Thanks to @gabrieldutra realized that the mobile version is crappy.
Updated a very simple fixed position header with minimal dropdown menu.

arikfr · 2019-10-15T15:08:08Z

Quick things I noticed:

The on-hover background color of menu items is hardly visible on my monitor.
When clicking on queries/dashboards in some cases it both navigates and open the dropdown menu. In the current version there is a clear distinction between the hit area for the dropdown and for the link.
In the current version when a menu open, the arrow points upwards.

gabrieldutra

I haven't tested the whole thing yet, but from my experience so far I can tell that mobile experience is a lot better 😁:

Navigation is much simpler (no more sub-sub-menus)
Now it closes the dropdown when you select an option
It's got position: fixed, so it's easily accessible from anywhere in page

For the desktop version I liked the blue bottom border and wdyt of having a black text color instead? (That gray feels de-emphasized to me)

gabrieldutra · 2019-10-15T15:24:35Z

client/app/components/app-header/AppHeader.jsx

+        </Dropdown>
+      </div>
+      <div className="header-logo">
+        <a href={clientConfig.basePath}>


Suggested change

<a href={clientConfig.basePath}>

<a href="./">

It annoys me that this depends on a backend config and it doesn't fit our Redash-preview case, for example... I wonder if the above works for Multi-org 🤔

That actually works for multi-org!
Learned sth new today :)

gabrieldutra · 2019-10-15T15:28:38Z

client/app/components/app-header/components/FavoritesDropdown.jsx

+    fetch().$promise.then(({ results }) => {
+      setLoading(false);
+      setItems(results);
+    });


It worths using .finally(() => setLoading(false)), to avoid breaking in this case:

Very true. Fixed!

ranbena · 2019-10-15T16:01:25Z

10x @arikfr

The on-hover background color of menu items is hardly visible on my monitor.

(It's the default ant-design dropdown menu color that we have across the site)
I strengthened it now - lmk if suitable.

When clicking on queries/dashboards in some cases it both navigates and open the dropdown menu. In the current version there is a clear distinction between the hit area for the dropdown and for the link.

I had to do some cunning css to recreate the way it is in the current version.
Now made it more accurate - lmk if it still happens.

In the current version when a menu open, the arrow points upwards.

Personally, I don't see the point in it.
But recreated it now. Check it out.

ranbena · 2019-10-15T16:13:30Z

For the desktop version I liked the blue bottom border and wdyt of having a black text color instead? (That gray feels de-emphasized to me)

@gabrieldutra I was waiting for @arikfr to say sth about it 😄
I kinda like the gray as it doesn't take the focus from the actual app content, but let's agree on #333? 4508544

gabrieldutra · 2019-10-15T23:04:03Z

client/app/components/app-header/AppHeader.jsx

+          {currentUser.isAdmin && (
+            <Menu.Item key="settings">
+              <a href="data_sources" className="menu-item-button">
+                <i className="fa fa-sliders" />


Considering the Help Trigger has a Tooltip on its own, wdyt about adding a Tooltip here as well? So will be a "standard" Icon Button behavior

Done. Also converted it to <Button> so it has the cool click animation.

gabrieldutra · 2019-10-16T00:00:55Z

I kinda like the gray as it doesn't take the focus from the actual app content, but let's agree on #333?

It does look like something Arik would say 😂. I prefer black, but #333333 doesn't bother me.

arikfr

Looks good 👍

I agree about the color of the text. While the idea of making it take less focus is valid it still looked pale.
We might want to add some caching to favorites, so it doesn't fetch on every click.

arikfr · 2019-10-16T17:36:31Z

Can do caching but what would cache bust? (assuming time based is a bad idea)

Why? Some short time span (1 minute?) will help with making sure that repeated clicks on the menu don't require new data.

ranbena · 2019-10-16T17:49:38Z

Why? Some short time span (1 minute?) will help with making sure that repeated clicks on the menu don't require new data.

Cuz if a user adds or removes faves, the fave menu would be out of date until a minute has passed. I think that's no good.
Is the concern perf optimization? The UX is such that the fave list loads immediately and updates on server response (it's not optimal but it works).

arikfr · 2019-10-16T17:56:17Z

Is the concern perf optimization?

Of course. I didn't look too much into it, but in a single check I saw that one of the favs endpoint returned results in 300ms and the other in 700ms. This feels borderline acceptable, no? Specially for repeated presses.

The other option is a Favorties "service" class, that holds current state and has a simple method of reset which clears the cache. This can be called by the Favorites control where we currently invoke the event.

ranbena · 2019-10-16T18:03:34Z

Of course. I didn't look too much into it, but in a single check I saw that one of the favs endpoint returned results in 300ms and the other in 700ms. This feels borderline acceptable, no? Specially for repeated presses.

Borderline acceptable how? The user wait time is 0 on repeated presses.

The other option is a Favorties "service" class, that holds current state and has a simple method of reset which clears the cache. This can be called by the Favorites control where we currently invoke the event.

Sure sounds good. Ok to implement this in a subsequent PR? (Don't want the PR to balloon 🎈)

ranbena · 2019-10-16T18:06:30Z

Correction - there IS a wait, but only when there're no faves in the list.

client/app/components/app-header/AppHeader.jsx

arikfr · 2019-10-16T19:19:54Z

Correction - there IS a wait, but only when there're no faves in the list.

Why?

If the values are memoized, then it's fine to leave as is.

ranbena · 2019-10-16T19:33:47Z

Why?

You're right I changed it now.

If the values are memoized, then it's fine to leave as is.

👍

Attended to comments

gabrieldutra · 2019-10-16T19:46:55Z

client/app/components/app-header/AppHeader.jsx

+  $route.reload();
+}
+
+function Desktop() {


Although implicit (and it's not a blocker for this) I would prefer naming those DesktopHeader and MobileHeader (or Navbar 🙂).

+1 for this 🙂

client/app/components/app-header/AppHeader.jsx

gabrieldutra

Looking forward to share to my friends such improvements in Mobile UX :)

Code looks great and I didn't find any issues in UI. Only thing is the favorites loading that is a bit annoying, which will be fixed soon. I left one last comment.

Thanks, Ran! 🎉

client/app/components/app-header/app-header.html

client/app/components/app-header/components/FavoritesDropdown.jsx

kravets-levko

New header looks even better than previous one (especially on mobile) 🎉

Few comments, no blockers at all (up to you to accept them or not 👍)

client/app/components/app-header/AppHeader.jsx

client/app/components/app-header/components/FavoritesDropdown.jsx

kravets-levko · 2019-10-18T19:38:09Z

client/app/components/app-header/AppHeader.jsx

+  $route.reload();
+}
+
+function Desktop() {


+1 for this 🙂

Co-Authored-By: Gabriel Dutra <[email protected]>

Co-Authored-By: Levko Kravets <[email protected]>

kravets-levko · 2019-10-18T20:00:27Z

Linter crashes for a some reason 🤔

AppHeader React Migration

f2f8db8

ranbena self-assigned this Oct 15, 2019

ranbena added 8 commits October 15, 2019 10:20

Fixed tests

4bd3461

Fixed test

91b3ab9

Responsive layout

e55125f

Better url templating code

a3aa95b

Merge branch 'master' into app-header

8b5ea0c

Fixed header height

ad2f19c

Fixed header height

962c631

Mobile version

b0c2894

Removed remaining old files

536d0da

gabrieldutra reviewed Oct 15, 2019

View reviewed changes

ranbena added 3 commits October 15, 2019 18:59

Changed menu item hover bg color

9115e9a

Fixed button click area

2d25cd3

Caret rotate animation

fc5a9f5

Fixed comments from Gabriel

66fc55a

ranbena added 3 commits October 15, 2019 20:09

Fixed test

7b3fbf7

Darkened text color

4508544

Style change for favorite dropdown

a64dfe9

gabrieldutra reviewed Oct 15, 2019

View reviewed changes

ranbena added 4 commits October 16, 2019 09:27

Alignment adjustments

1c2904a

Fixed hover colors

426e0c1

Merge branch 'master' into app-header

bedfa07

Changed onClick to onMouseUp for “New Dashboard”

a50af02

arikfr approved these changes Oct 16, 2019

View reviewed changes

ranbena force-pushed the app-header branch from 9df0605 to 853eca3 Compare October 16, 2019 15:21

Last fixes before publish

97bb08e

ranbena marked this pull request as ready for review October 16, 2019 18:51

arikfr reviewed Oct 16, 2019

View reviewed changes

client/app/components/app-header/AppHeader.jsx Outdated Show resolved Hide resolved

ranbena added 2 commits October 16, 2019 22:31

Renamed DropdownFavorites

c76a2c9

Less jumpy item rendering

ef77bef

ranbena changed the title ~~[WIP] AppHeader React Migration~~ App Header React migration Oct 16, 2019

gabrieldutra reviewed Oct 16, 2019

View reviewed changes

ranbena added 3 commits October 16, 2019 23:25

Swapped navigateTo for href

275c660

Merge remote-tracking branch 'origin/master' into app-header

eec5e3e

Improved layout responsiveness

dca65c7

gabrieldutra approved these changes Oct 18, 2019

View reviewed changes

client/app/components/app-header/app-header.html Show resolved Hide resolved

client/app/components/app-header/components/FavoritesDropdown.jsx Outdated Show resolved Hide resolved

kravets-levko approved these changes Oct 18, 2019

View reviewed changes

ranbena and others added 2 commits October 18, 2019 22:43

Update client/app/components/app-header/components/FavoritesDropdown.jsx

fdd2f05

Co-Authored-By: Gabriel Dutra <[email protected]>

Update client/app/components/app-header/AppHeader.jsx

acce095

Co-Authored-By: Levko Kravets <[email protected]>

ranbena added 3 commits October 19, 2019 01:06

react2angular removed from HelpTrigger

bf2306e

Renamed Desktop/Mobile, fixed substring syntax

82834b7

Added cache bust to fetchItems

816da58

ranbena merged commit a2e21dd into master Oct 19, 2019

ranbena deleted the app-header branch October 19, 2019 11:26

weekly-digest bot mentioned this pull request Oct 21, 2019

Weekly Digest (14 October, 2019 - 21 October, 2019) #4271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App Header React migration #4245

App Header React migration #4245

ranbena commented Oct 15, 2019 •

edited

Loading

ranbena commented Oct 15, 2019

arikfr commented Oct 15, 2019

gabrieldutra left a comment

gabrieldutra Oct 15, 2019

ranbena Oct 15, 2019

gabrieldutra Oct 15, 2019

ranbena Oct 15, 2019

ranbena commented Oct 15, 2019

ranbena commented Oct 15, 2019 •

edited

Loading

gabrieldutra Oct 15, 2019

ranbena Oct 16, 2019

gabrieldutra commented Oct 16, 2019

arikfr left a comment

arikfr commented Oct 16, 2019

ranbena commented Oct 16, 2019

arikfr commented Oct 16, 2019

ranbena commented Oct 16, 2019

ranbena commented Oct 16, 2019

arikfr commented Oct 16, 2019

ranbena commented Oct 16, 2019

gabrieldutra Oct 16, 2019 •

edited

Loading

kravets-levko Oct 18, 2019

gabrieldutra left a comment

kravets-levko left a comment

kravets-levko Oct 18, 2019

kravets-levko commented Oct 18, 2019

App Header React migration #4245

App Header React migration #4245

Conversation

ranbena commented Oct 15, 2019 • edited Loading

TODO

Changes

Manually tested

ranbena commented Oct 15, 2019

arikfr commented Oct 15, 2019

gabrieldutra 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

ranbena commented Oct 15, 2019

ranbena commented Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra commented Oct 16, 2019

arikfr left a comment

Choose a reason for hiding this comment

arikfr commented Oct 16, 2019

ranbena commented Oct 16, 2019

arikfr commented Oct 16, 2019

ranbena commented Oct 16, 2019

ranbena commented Oct 16, 2019

arikfr commented Oct 16, 2019

ranbena commented Oct 16, 2019

gabrieldutra Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra left a comment

Choose a reason for hiding this comment

kravets-levko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kravets-levko commented Oct 18, 2019

ranbena commented Oct 15, 2019 •

edited

Loading

ranbena commented Oct 15, 2019 •

edited

Loading

gabrieldutra Oct 16, 2019 •

edited

Loading