refactor: in left menu, move recently viewed from top to middle #87

yuye-aws · 2023-08-16T10:06:19Z

Description

The category name has changed from "recently viewed" to "recently visited". We need to confirm with UX team whether to render this category when there is no recently accessed items.

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter · 2023-08-16T13:41:40Z

Codecov Report

Merging #87 (3261510) into workspace (b49aa1f) will increase coverage by 0.00%.
Report is 3 commits behind head on workspace.
The diff coverage is 90.00%.

❗ Current head 3261510 differs from pull request most recent head 40421c9. Consider uploading reports for the commit 40421c9 to get more accurate results

@@            Coverage Diff             @@
##           workspace      #87   +/-   ##
==========================================
  Coverage      65.78%   65.78%           
==========================================
  Files           3334     3334           
  Lines          64441    64439    -2     
  Branches       10258    10256    -2     
==========================================
+ Hits           42390    42392    +2     
+ Misses         19482    19477    -5     
- Partials        2569     2570    +1

Flag	Coverage Δ
Linux_1	`34.71% <20.00%> (+<0.01%)`	⬆️
Linux_3	`42.66% <20.00%> (+<0.01%)`	⬆️
Windows_1	`34.72% <20.00%> (+<0.01%)`	⬆️
Windows_2	`54.69% <90.00%> (+0.02%)`	⬆️
Windows_3	`42.67% <20.00%> (+<0.01%)`	⬆️
Windows_4	`34.76% <20.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/core/public/chrome/nav_links/nav_link.ts	`66.66% <ø> (ø)`
src/core/public/chrome/ui/header/nav_link.tsx	`60.86% <66.66%> (+10.86%)`	⬆️
...c/core/public/chrome/ui/header/collapsible_nav.tsx	`93.75% <100.00%> (+5.87%)`	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

wanglam · 2023-08-16T14:40:48Z

src/core/public/chrome/ui/header/collapsible_nav.tsx

-              className="osdCollapsibleNav__recentsListGroup"
-            />
-          ) : (
-            <EuiText size="s" color="subdued" style={{ padding: '0 8px 8px' }}>


Will we still show No recently viewed items after this refactor?

No. This edge case for recently visited and favorites needs to be confirmed with UX.

Just get to know that No recently viewed items need to show. I will add it later.

wanglam · 2023-08-16T14:50:00Z

src/core/public/chrome/ui/header/nav_link.tsx

@@ -134,16 +131,10 @@ export function createRecentNavLink(

  return {
    href,
-    label,
+    baseUrl: href,


Is it a correct baseUrl value?

I think baseUrl can be considered as a placeholder rather than a real value. The reason is that Recent ChromeNavLink is only consumed by function createEuiListItem, where baseUrl is not used.

OK. Can we add some annotations about that? It may be confused.

baseUrl is supposed to be the base route used to open the root of an application, in this case, we are trying to generate ChromeNavLink from recently accessed url and ofc we don't know what's the root of an application.

Instead of simply making an conclusion:

Recent ChromeNavLink is only consumed by function createEuiListItem, where baseUrl is not used.

Here is what I'd suggest:

// src/core/public/chrome/ui/header/nav_link.tsx export type RecentNavLink = Omit<ChromeNavLink, 'baseUrl'>; export function createRecentChromeNavLink(/*....*/): RecentNavLink { //... } // src/core/public/chrome/ui/header/collapsible_nav.tsx const navLinks = useObservable(observables.navLinks$, []).filter((link) => !link.hidden); const allNavLinks: Array<ChromeNavLink | RecentNavLink> = [...navLinks]; const recentlyAccessed = useObservable(observables.recentlyAccessed$, []); if (recentlyAccessed.length) { allNavLinks.push( ...recentlyAccessed.map((link) => createRecentChromeNavLink(link, navLinks, basePath)) ); } else { allNavLinks.push(emptyRecentlyVisited); }

Do we need another type for const emptyRecentlyVisited?

I think you can use RecentNavLink I suggested.

Since baseUrl prop in navLinks is also not consumed, how about declaring the type as CollapsibleNavLink and then convert navLinks into this type?

// src/core/public/chrome/ui/header/nav_link.tsx export type CollapsibleNavLink = Omit<ChromeNavLink, 'baseUrl'>; // src/core/public/chrome/ui/header/collapsible_nav.tsx const navLinks = useObservable(observables.navLinks$, []).filter((link) => !link.hidden) .map(({ baseUrl, ...collapsibleNavLinkProps }) => collapsibleNavLinkProps);

Not necessary, I would suggest you keep the existing code as it is if you can.

Ok, I define a new type ChromeOrRecentNavLink in my latest commit to refactor the code. Please take a look

// src/core/public/chrome/ui/header/nav_link.tsx export type ChromeOrRecentNavLink = ChromeNavLink | RecentNavLink; // src/core/public/chrome/ui/header/collapsible_nav.tsx function getAllCategories(allCategorizedLinks: Record<string, ChromeOrRecentNavLink[]>) { ... }

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

ruanyl · 2023-08-18T09:09:58Z

src/core/public/chrome/ui/header/nav_link.tsx

@@ -110,12 +109,11 @@ export interface RecentNavLink {
 * @param navLinks
 * @param basePath
 */
-export function createRecentNavLink(
+export function createRecentChromeNavLink(


I'm still looking at the this, please don't merge for now, having a few doubts need to figure out

ruanyl · 2023-08-18T10:12:48Z

src/core/public/chrome/ui/header/nav_link.tsx

+  id: '',
+  disabled: true,
+  category: recentlyVisitedCategory,
+  title: i18n.translate('core.ui.EmptyRecentlyVisitied', {


Typo, Visited

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

ruanyl

Nice work!

* refactor recently visited links to category Signed-off-by: yuye-aws <yuyezhu@amazon.com> * bring back external link logic Signed-off-by: yuye-aws <yuyezhu@amazon.com> * add no recently visited items when empty Signed-off-by: yuye-aws <yuyezhu@amazon.com> * change annotation Signed-off-by: yuye-aws <yuyezhu@amazon.com> * refactor with type RecentNavLink Signed-off-by: yuye-aws <yuyezhu@amazon.com> * rename navlink type from ChromeOrRecentNavLink to CollapsibleNavLink Signed-off-by: yuye-aws <yuyezhu@amazon.com> --------- Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws added the Skip-Changelog label Aug 16, 2023

yuye-aws changed the title ~~refactor: in left menu, move recently viewed from top to a category between home and library~~ refactor: in left menu, move recently viewed from top to middle Aug 16, 2023

yuye-aws requested review from wanglam and SuZhou-Joe August 16, 2023 10:10

github-actions bot added the valued-contributor label Aug 16, 2023

wanglam reviewed Aug 16, 2023

View reviewed changes

yuye-aws added 4 commits August 17, 2023 08:16

refactor recently visited links to category

b0ed786

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

bring back external link logic

750a3b9

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

add no recently visited items when empty

0569e80

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

change annotation

5ed0c86

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

yuye-aws requested a review from wanglam August 18, 2023 04:19

wanglam approved these changes Aug 18, 2023

View reviewed changes

yuye-aws requested a review from ruanyl August 18, 2023 06:03

ruanyl reviewed Aug 18, 2023

View reviewed changes

yuye-aws added 2 commits August 21, 2023 11:30

refactor with type RecentNavLink

3261510

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

rename navlink type from ChromeOrRecentNavLink to CollapsibleNavLink

40421c9

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

ruanyl approved these changes Aug 21, 2023

View reviewed changes

yuye-aws merged commit 7fd4139 into ruanyl:workspace Aug 21, 2023
37 of 39 checks passed

yuye-aws deleted the recently branch August 21, 2023 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: in left menu, move recently viewed from top to middle #87

refactor: in left menu, move recently viewed from top to middle #87

yuye-aws commented Aug 16, 2023 •

edited

Loading

codecov-commenter commented Aug 16, 2023 •

edited

Loading

wanglam Aug 16, 2023

yuye-aws Aug 17, 2023 •

edited

Loading

yuye-aws Aug 17, 2023

wanglam Aug 16, 2023

yuye-aws Aug 17, 2023

wanglam Aug 17, 2023

yuye-aws Aug 18, 2023

ruanyl Aug 18, 2023 •

edited

Loading

yuye-aws Aug 18, 2023

ruanyl Aug 18, 2023 •

edited

Loading

yuye-aws Aug 18, 2023 •

edited

Loading

ruanyl Aug 21, 2023

yuye-aws Aug 21, 2023 •

edited

Loading

ruanyl Aug 18, 2023

ruanyl Aug 18, 2023

ruanyl left a comment

refactor: in left menu, move recently viewed from top to middle #87

refactor: in left menu, move recently viewed from top to middle #87

Conversation

yuye-aws commented Aug 16, 2023 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Aug 16, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

yuye-aws Aug 17, 2023 • edited Loading

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

ruanyl Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

yuye-aws Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuye-aws Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl left a comment

Choose a reason for hiding this comment

yuye-aws commented Aug 16, 2023 •

edited

Loading

codecov-commenter commented Aug 16, 2023 •

edited

Loading

yuye-aws Aug 17, 2023 •

edited

Loading

ruanyl Aug 18, 2023 •

edited

Loading

ruanyl Aug 18, 2023 •

edited

Loading

yuye-aws Aug 18, 2023 •

edited

Loading

yuye-aws Aug 21, 2023 •

edited

Loading