Open in new tab #856

alok-traceable · 2021-05-18T10:05:20Z

Description

Creating component Open In a new tab which basically wraps link component and icon component.
Adding support of query params in External URL navigator.
Adding utility to convert string dictionary to URL query param string and vice versa.

Testing

Unit test cases

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

codecov · 2021-05-18T10:09:23Z

Codecov Report

Merging #856 (b856073) into main (cadf4b6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #856   +/-   ##
=======================================
  Coverage   85.36%   85.36%           
=======================================
  Files         799      799           
  Lines       16428    16434    +6     
  Branches     2062     2064    +2     
=======================================
+ Hits        14023    14029    +6     
  Misses       2373     2373           
  Partials       32       32

Impacted Files	Coverage Δ
...ojects/common/src/navigation/navigation.service.ts	`93.10% <100.00%> (+0.72%)`	⬆️
...s/src/open-in-new-tab/open-in-new-tab.component.ts	`100.00% <100.00%> (ø)`
...ents/src/open-in-new-tab/open-in-new-tab.module.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cadf4b6...b856073. Read the comment docs.

projects/components/src/open-in-new-tab/open-in-new-tab.component.ts

projects/common/src/utilities/url/url-utilities.ts

projects/common/src/utilities/url/url-utilities.spec.ts

projects/common/src/navigation/navigation.service.ts

projects/common/src/utilities/url/url-utilities.ts

aaron-steinfeld · 2021-05-18T12:02:58Z

projects/common/src/time/time-range.service.ts

    const timeRange = this.getCurrentTimeRange();
    const fixedTimeRange: FixedTimeRange = TimeRangeService.toFixedTimeRange(timeRange.startTime, timeRange.endTime);
    const newTimeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${fixedTimeRange.toUrlString()}`;

    return this.navigationService.getAbsoluteCurrentUrl().replace(timeRangeParam, newTimeRangeParam);
  }

+  public getTimeRangeParams(): Dictionary<string> {


Does this need to be public?

We need the time range param to generate the new tab URL

package.json

projects/common/src/utilities/url/url-utilities.ts

projects/components/src/open-in-new-tab/open-in-new-tab.component.ts

aaron-steinfeld · 2021-05-19T12:15:11Z

projects/components/src/open-in-new-tab/open-in-new-tab.component.ts

    </div>
  `
 })
 export class OpenInNewTabComponent {
  @Input()
-  public size?: ButtonSize = ButtonSize.Small;
+  public paramsOrUrl?: ExternalNavigationParamsNew | string;


Do we still have the same default behavior of the current url?

aaron-steinfeld · 2021-05-19T12:25:47Z

projects/common/src/navigation/navigation.service.ts

@@ -80,11 +82,16 @@ export class NavigationService {

    if (params.navType === NavigationParamsType.External) {
      // External
+      const queryParamString = getQueryParamStringFromObject(params.queryParams ?? {});


It might be better to let angular do any url building for us (such as url tree). This would break if we passed it a a url that contained query params, since it wouldn't merge. Say http://website.com?param1=value and a second param via query params { param2=value }, we'd get http://website.com?param1=value?param2=value instead of http://website.com?param1=value&param2=value.

I think you can combine router.parseUrl, router.serializeUrl and maybe router.createUrlTree to get all the right behavior here.

aaron-steinfeld · 2021-05-19T12:37:53Z

projects/common/src/time/time-range.service.ts

    const timeRange = this.getCurrentTimeRange();
    const fixedTimeRange: FixedTimeRange = TimeRangeService.toFixedTimeRange(timeRange.startTime, timeRange.endTime);
    const newTimeRangeParam = `${TimeRangeService.TIME_RANGE_QUERY_PARAM}=${fixedTimeRange.toUrlString()}`;

    return this.navigationService.getAbsoluteCurrentUrl().replace(timeRangeParam, newTimeRangeParam);
  }

+  public getTimeRangeParams(): Params {


So the fuller picture is coming together here for me - I'm guessing this was left public to later use as query params in an open in new tab case that should maintain the time range, and that's also why queryParams handling was added in nav service. The problem here is we do not want callers to be concerned about this. Time range is not the only global param that needs to be preserved, and we don't want callers to even have to be aware of global params by design.

My suggestion would be to

revert the changes here (or at least make this method private)

update NavigationService.buildNavigationParams to use buildQueryParam for relative external urls, similar to what we do in navigateWithinApp

remove queryParams from ExternalNavigationParamsNew - it's never used today and shouldn't be for this use case, so no reason to build logic for it

anandtiwary · 2021-05-19T16:43:21Z

projects/components/src/open-in-new-tab/open-in-new-tab.component.ts

    </div>
  `
 })
 export class OpenInNewTabComponent {
  @Input()
-  public size?: ButtonSize = ButtonSize.Small;
+  public paramsOrUrl?: ExternalNavigationParamsNew | string;


Because this component is named as OpenInNewTab, i feel it should be able to take bothInApp and External params or a url and build a local External Param object. This would mean that no matter what url or params you pass, this component would always open it in a new tab. I think we can build the local param inside ngOnchanges with reasonable defaults for missing properties.

Could we please rename ExternalNavigationParamsNew to ExternalNavigationParams ?

thoughts? cc @aaron-steinfeld

Agree with the rename - how did that even get in?!

Also, yes - I think ideally the open in new tab component should be able to support and convert inApp params to external ones, but I personally don't feel it needs to hold up this PR, since we can add it later with fully backwards compatible support as it's a union type already.

I missed it when i was migrating to the new interface. But you approved the PR, so....

Sure we can make #1 change separately

aaron-steinfeld

lgtm other than those two leftovers

aaron-steinfeld · 2021-05-20T16:56:17Z

projects/common/src/navigation/navigation.service.ts

@@ -73,6 +73,22 @@ export class NavigationService {
    return this.currentParamMap.getAll(parameterName);
  }

+  public constructExternalUrl(urlString: string): string {


thanks! looks good - with these changes, do we still need the changes in time range service or the new url util?

nope, I already reverted those changes.

oh right, the TR ones are gone - one of the url utils looks like it's still there being unused though

anandtiwary · 2021-05-22T06:35:50Z

projects/common/src/utilities/url/url-utilities.ts

+import { HttpParams } from '@angular/common/http';
+import { Params } from '@angular/router';
+
+export const getQueryParamStringFromObject = (params: Params): string =>


Is this needed or should we use the below function directly?

Okay I will remove this function as it is not being used anywhere, but It is better to have a separate function like this, firstly consumers didn't have to write a new instance of HttpParams every time, and secondly if later we want to standardize queryParams key-value pair we need to change this function only.

aaron-steinfeld

Thanks @alok-traceable

github-actions · 2021-05-25T02:28:15Z

Unit Test Results

    4 files ±0 254 suites ±0 15m 28s ⏱️ +17s
916 tests +2 916 ✔️ +2 0 💤 ±0 0 ❌ ±0
922 runs +2 922 ✔️ +2 0 💤 ±0 0 ❌ ±0

Results for commit 682e80c. ± Comparison against base commit cadf4b6.

Alok Singh added 3 commits May 17, 2021 15:46

feature: adding open in new tab icon on security overlay

85adc37

feat: changes for open in new tab component

20087d1

fix: file rename and unti tests

6dfa494

alok-traceable requested a review from a team as a code owner May 18, 2021 10:05

alok-traceable assigned arjunlalb and anandtiwary May 18, 2021

This comment has been minimized.

Sign in to view

Alok Singh added 2 commits May 18, 2021 19:17

fix: lint fixes

aa34791

fix: lint fixes

b9e31b7

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed May 18, 2021

View reviewed changes

anandtiwary reviewed May 18, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

arjunlalb reviewed May 18, 2021

View reviewed changes

projects/common/src/utilities/url/url-utilities.ts Outdated Show resolved Hide resolved

projects/common/src/utilities/url/url-utilities.ts Outdated Show resolved Hide resolved

projects/components/src/open-in-new-tab/open-in-new-tab.component.ts Outdated Show resolved Hide resolved

Alok Singh added 3 commits May 19, 2021 11:17

fix: pr suggested changes

4266e8b

fix: pr suggested changes

6169311

fix: lint fixes

101e0ce

This comment has been minimized.

Sign in to view

fix: pr suggested changes

532fe1f

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed May 19, 2021

View reviewed changes

anandtiwary reviewed May 19, 2021

View reviewed changes

feat: pr suggested changes

65f9181

This comment has been minimized.

Sign in to view

feat: pr suggested changes

98f65cd

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed May 20, 2021

View reviewed changes

fix: merging main

f7107cc

This comment has been minimized.

Sign in to view

fix: merging main

f59b14f

This comment has been minimized.

Sign in to view

fix: imporving code coverage

8ff748c

This comment has been minimized.

Sign in to view

anandtiwary reviewed May 22, 2021

View reviewed changes

fix: removing unused utilities

b5d74ba

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes May 24, 2021

View reviewed changes

anandtiwary approved these changes May 24, 2021

View reviewed changes

Merge branch 'main' into open_in_new_tab

b856073

This comment has been minimized.

Sign in to view

alok-traceable merged commit 682e80c into main May 25, 2021

alok-traceable deleted the open_in_new_tab branch May 25, 2021 02:23

Open in new tab #856

Open in new tab #856

Uh oh!

Conversation

alok-traceable commented May 18, 2021

Description

Testing

Checklist:

Uh oh!

codecov bot commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anandtiwary May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

alok-traceable May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions bot commented May 25, 2021

Unit Test Results

Uh oh!

Uh oh!

codecov bot commented May 18, 2021 •

edited

Loading

anandtiwary May 19, 2021 •

edited

Loading

alok-traceable May 24, 2021 •

edited

Loading