[#1894] Add embeddable ramp widget #1988

MarcusTXK · 2023-04-08T16:04:49Z

Proposed commit message

The ability to copy a link for an embeddable ramp widget with the click 
of a button and embed the ramp widget into any existing website will be 
a useful feature. This allows users who intend to feature any 
contributions made to a repository/projects in their portfolio/any 
website, especially as I feel it is both eye-catching and provides a 
lot of useful information. The embedded ramps should follow any filters 
configured by the users above so that the information displayed by the 
ramps can be customized. 

Let's add a button to allow users to copy and embed widgets.

Other information

Screenshots

RepoSense Report

Embedded Group Ramp (Unmerged)

Note that elements such as the numbering and certain buttons that do not have usage in iframes are hidden. An additional button that redirects the user to the RepoSense report with the current inputs is added to each user.

Embedded Group Ramp (Merged)

Embedded Single Ramps

Similarly here, the numbering and unnecessary buttons are removed.

The height of the iframe are calculated based on the number of charts in the iframe * chart height + title (if present) such that no scrolling of the iframe is required when viewing it. The width is set to an arbitrary value of 800. These dimensions can be edited by the user in the iframe should they prefer other values.

Tested the iframes on the following html file (green bg to see the widget margins better) :

<!DOCTYPE html>
<html>
<head>
	<title>Example with iFrame:</title>
</head>

<style>
body {
  background-color: #7bc96f;
}
</style>
<body>
	<h1>Embedded Widget</h1>
        <iframe src="XXX" frameBorder="0" width="800px" height="XXXpx"></iframe>
</html>

Design Decisions

Why iframes?

For embeddable widgets I initially considered two options, the first being standard Web Components that can be embedded in any HTML or iframes.

I ultimately decided to use iframes as using standard Web Components required prebuilding of the components and for users to add two items, a link in the head for css and a script in the body for the widget source, which is troublesome for the user. Iframes on the otherhand can be directly embedded in the body of any html file with no hassle.

Reuse of `c-summary-charts.vue`

I initially intended to create a seperate widget component, however this has the drawback of being hard to maintain as if the summary chart is updated, the widget would have to be updated separately. Since c-summary-charts.vue renders the charts in a double for loop, I realized that we can easily reuse it but filter out non relevant elements and only render the specified charts. This can be thought of as a matrix, where we access the required row or required element directly by keeping track of indexes. This is done via the url parameters chartGroupIndex and chartIndex.

Overall

This are the main changes:

I have extracted out all common logic between c-home and c-widget in app.vue. The common logic are passed down as props to the respective pages.
router-view renders c-home or c-widget depending on if the routing is / or /#/widget.
c-summary does not render the summary-picker if isWidgetMode prop is passed in, and now passes in chartGroupIndex and chartIndex to c-summary-charts which only renders the specified indexes.
c-summary-chart conditionally renders certain buttons, depending on if it is currently a widget (which is determined by if chartGroupIndex and chartIndex are passed present).
Common CSS for summary chart between c-widget and c-home has been shifted into summary-chart.scss

This must be merged after #1974.
Note: I am merging into a branch with the same code as #1974 to make it easier for review. Once #1974 is merged in I will change it back to merging into master

tl;dr

Common logic moved into app, app renders either c-home or c-widget and passes in common logic as props.
c-widget renders c-summary with some extra props.

Edit: Documented bugs:

clicking ramp does not lead to report
non-localhost for local deployment does not work
warnings not hidden

vvidday

Thanks for your work on this @MarcusTXK! The structure looks great, just found some small issues/bugs

Currently clicking on the "view breakdown of commits" (the button that will usually open the commits panel on the report), the user is redirected to the report itself (with commits panel not open). I was wondering if it would be better to open the corresponding commits panel on the report as well?
Similarly, if granularity is set to "week" and the user clicks on the ramp, ordinarily on the report it opens the corresponding commit in the commits panel, but in the widget it does nothing. I think it would be good to minimally redirect to the report, and preferably open the corresponding commit like it does in the original report
Currently the widget renders any errors in the summary panel, could this be removed in widget view?

Screenshot

ckcherry23

Hi @MarcusTXK, thanks for working on this PR!
On some initial testing, I noticed a few things to address:

The copy to clipboard function works on localhost:8080 but not on the IP addresses (eg. http://0.0.0.0:8080/). I don't know what the issue is here, but can you take a look to ensure it works in production?
The tooltips on hover appear outside the viewport of the iframe. This can probably be fixed when resolving Add dynamic positioning support for tooltip #1929. However, the issue is more amplified on the iframes, as there is no way to see what's on the tooltips even after some scrolling.
As discussed in class, the embeddable ramp widget has some bugs when tested using the RepoSense backend test report data. Please take a look, thanks!

This is the code to the iframe I used, which is supposed to be the first summary chart on the report:
```
<iframe src="http://localhost:8080/#/widget/?search=&sort=groupTitle%20dsc&sortWithin=title&since=2018-03-01&timeframe=commit&mergegroup=&groupSelect=groupByRepos&breakdown=false&chartGroupIndex=0&chartIndex=0" frameBorder="0" width="800px" height="104px"></iframe>
```
Instead of just that one chart, the first summary chart in each of the repo is included in the iframe.

MarcusTXK · 2023-04-13T21:23:48Z

Thanks a lot for the feedback, I have addressed all of them below.

@vvidday

Currently the widget renders any errors in the summary panel, could this be removed in widget view?

Thanks for noticing this! I have fixed this as well as breakdown filetypes (that I realised was not hidden as well)

Currently clicking on the "view breakdown of commits" (the button that will usually open the commits panel on the report), the user is redirected to the report itself (with commits panel not open). I was wondering if it would be better to open the corresponding commits panel on the report as well?

I tried implementing this logic, however after spending quite awhile, I realized that it requires quite a bit of changing the code in order to render the URL for href as one that points to the open tab (which needs to be "computed" on render of c-summary-charts), as it involves invoking openTabZoom and replicating all of its logic as well as the other functions it calls, but without modifying the actual state (since we only want to get the url and not actually render what happens if it is clicked).

I think a simple workaround for now would be for the user to open the tab first, and then copy the iframe, if the user wants the page to be redirected to the report with their tab open. If we are to add logic for this, I feel it should be in a seperate PR as part of a further enhancement of the widget feature, as the non-trival refactoring needed will make this PR more confusing (as the current one already involves a degree of refactoring).

Similarly, if granularity is set to "week" and the user clicks on the ramp, ordinarily on the report it opens the corresponding commit in the commits panel, but in the widget it does nothing. I think it would be good to minimally redirect to the report, and preferably open the corresponding commit like it does in the original report

Good catch for this! For now, I have updated the logic to redirect to the report for ramps that are of granularity "week". I think this functionality can be added together with the one above in a separate PR, for the same reasons of adding even more complexity to this PR.

@ckcherry23

The copy to clipboard function works on localhost:8080 but not on the IP addresses (eg. http://0.0.0.0:8080/). I don't know what the issue is here, but can you take a look to ensure it works in production?

This was an interesting issue to solve. It turns out that navigator.clipboard requires a secure origin — either HTTPS or localhost (or disabled by running Chrome with a flag), otherwise it will be undefined. I have added a workaround for non-secure origins. I originally tried using other libraries such as copy-to-clipboard and Clipboard.js, but I found that they did not work either, so I decided to use document.execCommand('copy'), which unfortunately requires spawning of a temporary text area that is removed subsequently, but I think that this is fine as it is only for development.

The tooltips on hover appear outside the viewport of the iframe. This can probably be fixed when resolving #1929. However, the issue is more amplified on the iframes, as there is no way to see what's on the tooltips even after some scrolling.

Yes, I agree that this is an issue, but I feel that this is best fixed in a separate PR. Perhaps in widget mode I could temporarily disable tooltips until this is fixed.

As discussed in class, the embeddable ramp widget has some bugs when tested using the RepoSense backend test report data. Please take a look, thanks!

Thanks for noticing this bug, I did not notice this as the reports I tested it on were smaller. I have fixed this issue.

Additionally, @ckcherry23 brought up a good idea during our CS3281 meeting to add feedback to the user when the copy button is clicked, so I have modified the tooltip to now show "Copied iframe" upon clicking of the copy button, for 2 seconds, before it reverts back to its original text.

MarcusTXK · 2023-04-21T08:46:46Z

@HCY123902 Thanks for the feedback, Ive updated the icon to redirect the user to the RepoSense Report
I have also updated the widget to include the repository and branch title for Single Charts as well as Author for Group by Author Ramp.

Single Chart for group by Repo/Branch:

Single Chart for group by Author:

…b.com/MarcusTXK/RepoSense into 1894-add-embeddable-ramp-widget-split

docs/ug/sharingReports.md

frontend/src/components/c-summary-charts.vue

ckcherry23

Hi @MarcusTXK, the embedded iframe with the title looks cleaner and almost ready. I noticed a few more issues:

There is a tooltip for the 'number of lines' shown beside the title (for both group by author and group by repo). Can you disable this tooltip as well for widget mode?
It doesn't seem like the copy iframe feature works for ramp charts when using the None option for group by. I am not sure if you intended this, but I think we should allow embedding ramps even when using theNone option. Can you recheck or clarify this?

MarcusTXK · 2023-04-22T05:55:29Z

@ckcherry23 Thanks for helping to take a look, appreciate your help!

There is a tooltip for the 'number of lines' shown beside the title (for both group by author and group by repo). Can you disable this tooltip as well for widget mode?

Thanks for spotting this, I have hidden it for widget mode.

It doesn't seem like the copy iframe feature works for ramp charts when using the None option for group by. I am not sure if you intended this, but I think we should allow embedding ramps even when using theNone option. Can you recheck or clarify this?

This was an accidental bug introduced with the inclusion of title for single charts, and the title being undefined for group by None. Good catch, I have fixed this.

ckcherry23 · 2023-04-22T14:24:42Z

Thanks for spotting this, I have hidden it for widget mode.

I think there might still be an issue with the tooltip

ckcherry23

LGTM!

zhoukerrr · 2023-04-25T13:33:02Z

frontend/src/components/c-summary-charts.vue

        v-on:click="handleMergeGroup(getGroupName(repo))"
      )
        .tooltip
          font-awesome-icon.icon-button(:icon="['fas', 'chevron-up']")
          span.tooltip-text Click to merge group
      a(
-        v-if="isGroupMerged(getGroupName(repo))",
+        v-if="isGroupMerged(getGroupName(repo)) && !isChartGroupWidgetMode",


@MarcusTXK is it possible to group these repeated checks to a higher level? I know this is how it is currently done but we can look into how to reorganise to reduce repeated codes like this one.

Yeah I definitely agree it would be great if we could refactor these repeated individual checks into a common one, though I unfortunately cant think of a clean way to do it at the moment.

Lets explore how to refactor the frontend for this as well as other similar instances in a future issue.

zhoukerrr

LGTM

github-actions · 2023-04-30T02:11:18Z

The following links are for previewing this pull request:

Dashboard Preview: https://dashboard-1988-pr-reposense-reposense.surge.sh
Docs Preview: https://docs-1988-pr-reposense-reposense.surge.sh

MarcusTXK and others added 10 commits April 3, 2023 16:51

Add Vue Router and shift app.vue into home.vue

40e4d61

Fix selectedFiles not rendering properly bug

60571e5

Merge branch 'master' into 1971-add-vue-router

30f7e99

Fix mui-select.grouping not visible

63217d2

Refactor getFontColor back to global function

e2fd873

Merge branch 'master' into 1971-add-vue-router

acd9a5d

Shift global styles into app.vue

d4e5ee4

Shift common code from home into app and Add widget files

d81bb39

Add buttons to copy iframe and Add function to generate iframe

06b9235

Update getReportLink() to return link to report without widget mode

b5d5c08

MarcusTXK mentioned this pull request Apr 8, 2023

[#1894] Add embeddable ramp widget #1954

Closed

8 tasks

MarcusTXK changed the title ~~[#1894] Add embeddable ramp widget split~~ [#1894] Add embeddable ramp widget Apr 8, 2023

MarcusTXK added 2 commits April 9, 2023 00:21

Fix linting errors

b6fa7d8

Fix failing zoomView_switchZoom test

f9ada73

MarcusTXK marked this pull request as ready for review April 8, 2023 16:34

MarcusTXK requested a review from a team April 8, 2023 16:34

MarcusTXK added the s.ToReview label Apr 8, 2023

vvidday reviewed Apr 10, 2023

View reviewed changes

ckcherry23 reviewed Apr 10, 2023

View reviewed changes

MarcusTXK added 5 commits April 14, 2023 01:08

Add backup for clipboard copy for non secure origin

5bc2fd7

Update tooltip text on copy for user feedback when copied

94acf25

Update c-ramp onclick in widget mode

71dd7be

Fix error and breakdown filetypes showing in widget mode

6c3b5d0

Fix filtering bug

99dc19d

MarcusTXK requested review from ckcherry23 and vvidday April 13, 2023 21:23

MarcusTXK changed the base branch from 1971-add-vue-router to master April 13, 2023 21:26

MarcusTXK and others added 2 commits April 14, 2023 10:44

Fix linting errors

2037b25

Merge branch 'master' into 1894-add-embeddable-ramp-widget-split

f1d0501

MarcusTXK added 3 commits April 21, 2023 16:23

Update icon for redirect to report in widget

3a9c032

Remove active-background in widget mode

e3d26b9

Update for single charts to include title

16eeda0

MarcusTXK and others added 4 commits April 21, 2023 17:24

Update developer and user guide to add information on widget feature

a498f57

Merge branch 'master' into 1894-add-embeddable-ramp-widget-split

6ca63d6

Update naming of file in docs to be consistent with rest

48a7685

Merge branch '1894-add-embeddable-ramp-widget-split' of https://githu…

c7eb6fc

…b.com/MarcusTXK/RepoSense into 1894-add-embeddable-ramp-widget-split

HCY123902 requested changes Apr 21, 2023

View reviewed changes

docs/ug/sharingReports.md Outdated Show resolved Hide resolved

frontend/src/components/c-summary-charts.vue Show resolved Hide resolved

MarcusTXK added 2 commits April 21, 2023 18:42

Add import for _utils.scss

dc78e7d

Shift embeddable widgets section out

68ae501

MarcusTXK requested a review from HCY123902 April 21, 2023 10:46

HCY123902 approved these changes Apr 21, 2023

View reviewed changes

HCY123902 requested review from a team April 21, 2023 12:46

ckcherry23 reviewed Apr 22, 2023

View reviewed changes

MarcusTXK added 2 commits April 22, 2023 13:49

Fix undefined title for group by none

02f155a

Hide missed tooltip in widget mode

92417e5

MarcusTXK requested a review from ckcherry23 April 22, 2023 05:57

ckcherry23 requested a review from zhoukerrr April 22, 2023 14:44

Disabled missed tooltip for widget mode

9e2e271

ckcherry23 approved these changes Apr 24, 2023

View reviewed changes

zhoukerrr reviewed Apr 25, 2023

View reviewed changes

zhoukerrr approved these changes Apr 25, 2023

View reviewed changes

MarcusTXK added s.ToMerge and removed s.ToReview labels Apr 26, 2023

dcshzj merged commit 1cc3e67 into reposense:master Apr 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1894] Add embeddable ramp widget #1988

[#1894] Add embeddable ramp widget #1988

MarcusTXK commented Apr 8, 2023 •

edited

Loading

vvidday left a comment •

edited

Loading

ckcherry23 left a comment •

edited

Loading

MarcusTXK commented Apr 13, 2023

MarcusTXK commented Apr 21, 2023 •

edited

Loading

ckcherry23 left a comment

MarcusTXK commented Apr 22, 2023

ckcherry23 commented Apr 22, 2023

ckcherry23 left a comment

zhoukerrr Apr 25, 2023

MarcusTXK Apr 26, 2023 •

edited

Loading

zhoukerrr left a comment

github-actions bot commented Apr 30, 2023

[#1894] Add embeddable ramp widget #1988

[#1894] Add embeddable ramp widget #1988

Conversation

MarcusTXK commented Apr 8, 2023 • edited Loading

Proposed commit message

Other information

Screenshots

Design Decisions

Why iframes?

Reuse of c-summary-charts.vue

Overall

tl;dr

vvidday left a comment • edited Loading

Choose a reason for hiding this comment

ckcherry23 left a comment • edited Loading

Choose a reason for hiding this comment

MarcusTXK commented Apr 13, 2023

MarcusTXK commented Apr 21, 2023 • edited Loading

ckcherry23 left a comment

Choose a reason for hiding this comment

MarcusTXK commented Apr 22, 2023

ckcherry23 commented Apr 22, 2023

ckcherry23 left a comment

Choose a reason for hiding this comment

zhoukerrr Apr 25, 2023

Choose a reason for hiding this comment

MarcusTXK Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

zhoukerrr left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 30, 2023

MarcusTXK commented Apr 8, 2023 •

edited

Loading

Reuse of `c-summary-charts.vue`

vvidday left a comment •

edited

Loading

ckcherry23 left a comment •

edited

Loading

MarcusTXK commented Apr 21, 2023 •

edited

Loading

MarcusTXK Apr 26, 2023 •

edited

Loading