Link Popovers added #380

krshubham · 2017-06-26T15:32:05Z

Added the link popovers and updated the docker-compose.yml to auto download the docker image. Hope it works fine 😄

publish new email images

atfornes · 2017-06-26T22:15:24Z

Hi @krshubham, thanks for the pull request. Good work!

I have tried it and there are some things to fix:

Now we cannot change the link, because the popover is of the preview, even in edit mode. Either a change link should appear in the popover or only the edition options to be displayed in edit mode.
when title and description is not provided something different than undefinedand description not providedshould be provided.
Some style, giving more weight to title might improve the visual aspect. Please do a visual proposal and share it with @elenamv for feedback.

Thanks again for your work

atfornes · 2017-08-27T10:24:08Z

@krshubham, did you make the changes I proposed in my previous comment in this thread?

krshubham · 2017-08-27T14:16:57Z

@atfornes , I started out with the first issue, and it turned out that the issue was more related to a bug that prevailed before starting the task. Although, the popover covers over the link but when it goes we can open the link annotation dialog box. I checked that!. After some time on this, I started focusing on the SEO, since it itself was a bit complicated! However I'd mention these things in the gist that I am writing!

atfornes

Please, check this review in order to merge the code.

I am sorry it comes late.

atfornes · 2017-08-27T17:17:45Z

src/js/directives/pad.js

+       var annotations = {};
+
+       function openLinkPopover(event,range){
+        if(!styleAppended){


Style should be defined in a src/sass/*.sass file. In this case it could be either a new one or pad.sass

atfornes · 2017-08-27T17:18:58Z

src/js/directives/pad.js

+        }
+        timer = $timeout(() => {
+          event.stopPropagation();
+          let div =  document.createElement('div');


Html code should be provided in a template, unless there is a good reason not to do it. Feel free to add a new one for this popovers.

atfornes · 2017-08-27T17:24:21Z

src/js/services/pad/linkPreview.js

+  .factory('linkPreview', linkPreview);
+
+	function linkPreview($http) {
+    const LINK_PREVIEW_SERVER_URL = 'http://localhost:9090/fetch';


This address should be provided in a configuration file instead ;)

atfornes · 2017-08-27T17:34:04Z

src/js/directives/pad.js

-            // https://github.com/P2Pvalue/swellrt/issues/84
-            var editorElement = angular.element($element.find('.swellrt-editor').children()[0]);
+        $scope.padReady = function(editor) {
+          // FIXME


All FIXME comments should be removed if already fixed or transformed into a issue. The comment can stay in the later case providing a link to the actual issue.

atfornes · 2017-08-27T17:34:44Z

src/js/services/pad/linkPreview.js

+	function linkPreview($http) {
+    const LINK_PREVIEW_SERVER_URL = 'http://localhost:9090/fetch';
+    function getMetaData(url){
+      //TODO: implement a check for the URL to be correct


All TODOs shoud be transformed into issues or removed.

krshubham · 2017-08-28T07:43:43Z

Separated HTML and CSS in respective files
Added weight to the title
Checked indentation properly

atfornes · 2017-08-29T18:38:43Z

@krshubham, please share the code improvements you mentioned in your previous comment, are they ready?

atfornes

Some css style comments, I hope they help :)

atfornes · 2017-08-29T18:56:00Z

src/js/directives/pad.js

+            height: auto;
+            max-height: 40px;
+            margin: 0 auto;
+            overflow: auto;


I would chose a hidden overflow with text-overflow: ellipsis

Also, maybe max-height is needed as now only allows two lines.

atfornes · 2017-08-29T18:56:39Z

src/js/directives/pad.js

+            border-radius: 6px;
+          }
+          div.popover-link-image{
+            width: 330px;


I would rather use max-width and max-height not to brake image ratio

atfornes · 2017-08-29T18:58:50Z

src/js/directives/pad.js

+            overflow: hidden;
+            white-space: nowrap;
+          }
+          .popover-link-address{


maybe use the same green as links in the pad for popover links.

Actually, is it really needed to display the url? we can just have href to the url at title and description... maybe also image? can you please check what other webs do and copy?

atfornes · 2017-08-29T19:01:33Z

src/js/directives/pad.js

+            left: -1px;
+            border-style: solid;
+            border-width: 0 10px 10px;
+            border-color: #F1F1F1 transparent;


Please, use the colors at our src/sass/colors.sass file. Here and in every css color.

atfornes · 2017-08-29T19:03:28Z

src/js/directives/pad.js

+          div.style.backgroundColor = '#F2F2F2';
+          div.style.paddingTop = '5px';
+          div.id = 'popover-container';
+          document.body.appendChild(div);


Adding the popover inside the pad div breaks its dimensions when using narrow screens. I would try to get the position of the link and position it adding the element outside the editor element.

Kumar Shubham added 11 commits April 9, 2017 09:42

Merge pull request Grasia#1 from Grasia/master

06fd84c

publish new email images

gulpfile error handlers for pipes added

22bec23

Merge branch 'master' of https://github.com/krshubham/teem

21a3017

Added basic popover

87894a9

Merge remote-tracking branch 'upstream/master'

aa7fcce

Some changes in the popover

9dd4e63

Added some stable changes to popover

a5e7980

separating bug fixes

46b44a9

Popover added + gulpfile fix

83da6ea

Removed jshint warnings

16881d7

Added automatic pull of teemlp docker image

997258f

update secrets

188abec

atfornes requested changes Aug 27, 2017

View reviewed changes

krshubham closed this Aug 28, 2017

krshubham reopened this Aug 28, 2017

Merge branch 'master' into popover-final

092dc6f

some final corrections added

dada231

atfornes reviewed Aug 29, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link Popovers added #380

Link Popovers added #380

krshubham commented Jun 26, 2017

atfornes commented Jun 26, 2017

atfornes commented Aug 27, 2017

krshubham commented Aug 27, 2017

atfornes left a comment

atfornes Aug 27, 2017

atfornes Aug 27, 2017

atfornes Aug 27, 2017

atfornes Aug 27, 2017

atfornes Aug 27, 2017

krshubham commented Aug 28, 2017

atfornes commented Aug 29, 2017

atfornes left a comment

atfornes Aug 29, 2017

atfornes Aug 29, 2017

atfornes Aug 29, 2017

atfornes Aug 29, 2017

atfornes Aug 29, 2017

atfornes Aug 29, 2017

atfornes Aug 29, 2017

Link Popovers added #380

Are you sure you want to change the base?

Link Popovers added #380

Conversation

krshubham commented Jun 26, 2017

atfornes commented Jun 26, 2017

atfornes commented Aug 27, 2017

krshubham commented Aug 27, 2017

atfornes 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

krshubham commented Aug 28, 2017

atfornes commented Aug 29, 2017

atfornes 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