Popover final #383

krshubham · 2017-08-30T09:37:03Z

In this PR, respective html and css for the popover were added in the corresponding files.
Future work:

Improving the style of the popover with matching theme for Teem
Caching of meta data for different links to provide quick access to the og data on subsequent requests
Popover should not hide on mouseover

publish new email images

atfornes · 2017-08-31T11:18:07Z

src/js/directives/pad.js

+              let btn = event.target;
+              //cannot inject the spinner HTML directly here
+              let inHTML = `
+          <style>


Style should go to a sass file, thanks!

atfornes · 2017-08-31T11:18:31Z

src/js/directives/pad.js

+            margin: 150px 0;
+          }
+          </style>
+          <div class="pos-r">


Html should be written in a template file :)

atfornes · 2017-08-31T11:18:55Z

src/js/directives/pad.js

+              let clientRect = range.node.nextSibling ?
+                range.node.nextSibling.getBoundingClientRect() :
+                range.node.parentElement.getBoundingClientRect();
+              div.innerHTML = inHTML;


Style to sass file

I moved almost all the styles to sass only what seemed as necessary to me is kept here

atfornes · 2017-08-31T11:19:07Z

src/js/directives/pad.js

-                </svg>
-              </div>
-            </div>`;
+          <div class="pos-r">


html to templates

atfornes · 2017-08-31T11:19:57Z

src/js/directives/pad.js

+                    else {
+                      div.style.height = '110px';
+                      innerEle.innerHTML = document.getElementById('no-title-in-url').innerHTML;
+                      innerEle.querySelector('#popoverLinkTitle').innerHTML = urlTitle;


Also, why not use angular to populate the title, description, etc. with {{popover.title}} and similar?

Oh yes, I'll surely do that! Poor me!

It's not working, idk I checked this but it didn't seem to work. in pad.js I made $scope.popover = { }and then populated this object with popover data but in template when I wrote {{popover.title}} it rendered nothing! I didn't get any errors even!

atfornes · 2017-08-31T11:20:24Z

src/js/services/pad/linkPreview.js

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


Server URL to config file

Sure, i'll update the code

I need your help here ! @atfornes ! What would be the best way to do this?

atfornes · 2017-08-31T11:21:21Z

src/templates/pad.html

@@ -66,3 +66,41 @@
  </div>

 </div>
+
+<div style="display: none;">


Style in sass files is preferred over inline style. Thanks!

atfornes · 2017-09-01T14:36:47Z

src/sass/pad.sass

@@ -227,7 +227,7 @@ div
  position: absolute
  border: 1px solid $grey
  z-index: 3
-  background-color: lightgrey
+  background-color: darken(#fff, 5%)


why not to use one of our colors as $light-grey?

Its a bit more darker than what I wanted

The colors have been carefully chosen by our designer @elenamv to combine together. Please see if there is a lighter color in our selection that you can use, maybe $pale-blue?

atfornes · 2017-09-01T14:37:33Z

Sorry, I see now that some of the comments I did were not yet sent

atfornes

some comments, please check them @krshubham.

atfornes · 2017-09-04T10:13:18Z

gulpfile.js

@@ -220,7 +220,16 @@ var gulp           = require('gulp'),
  spawn          = require('child_process').spawn,
  gutil          = require('gulp-util');



changes in gulpfile should go to the pull request about error handling in gulp

I'll be fixing this soon! Sorry about this, I didn't check that I had updated gulpfile here

atfornes · 2017-09-04T10:16:06Z

src/templates/pad.html

+    <div id="popover">
+      <div class="popover-link-title" id="popoverLinkTitle"></div>
+      <div class="popover-link-image">
+        <img src="" alt="" height="180" width="330" id="popoverLinkImage">


please, remove inline css and bring it to style files

Sorry :( ! I'll push the changes soon.

atfornes · 2017-09-04T10:17:46Z

@krshubham, this pull request still have unattended comments, please answer them. When can we expect the updated pull request?

atfornes · 2017-09-05T10:24:53Z

a clean version of this pull request is merged at https://github.com/Grasia/teem/tree/krshubham-popover-final

Please @krshubham, open an issue that links to all the issues remaining in this branch. In order to merge, some things have to be still fixed, for instance, the popover hiding when the mouse is over it.

krshubham · 2017-09-05T12:55:49Z

Okay, I'll open an issue listing all the work that should be done ! 👍

Kumar Shubham added 17 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

Merge branch 'master' into popover-final

092dc6f

some final corrections added

dada231

Corrected issues related for pull#380

7b91689

Added max-width and max-height for popover image

cf09fb8

Moved css to pad.sass

4701fbe

Fixed color in pad sass

922e961

atfornes reviewed Sep 1, 2017

View reviewed changes

atfornes reviewed Sep 4, 2017

View reviewed changes

gulpfile restored to grasia/teem:master

e33c15c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Popover final #383

Popover final #383

krshubham commented Aug 30, 2017

atfornes Aug 31, 2017

atfornes Aug 31, 2017

atfornes Aug 31, 2017

krshubham Sep 1, 2017

atfornes Aug 31, 2017

atfornes Aug 31, 2017

krshubham Sep 1, 2017

krshubham Sep 4, 2017

atfornes Aug 31, 2017

krshubham Sep 1, 2017

krshubham Sep 4, 2017

atfornes Aug 31, 2017

atfornes Sep 1, 2017

krshubham Sep 1, 2017

atfornes Sep 1, 2017

atfornes commented Sep 1, 2017

atfornes left a comment

atfornes Sep 4, 2017

krshubham Sep 4, 2017

atfornes Sep 4, 2017

krshubham Sep 4, 2017

atfornes commented Sep 4, 2017

atfornes commented Sep 5, 2017

krshubham commented Sep 5, 2017 •

edited

Loading



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

		@@ -220,7 +220,16 @@ var gulp = require('gulp'),
		spawn = require('child_process').spawn,
		gutil = require('gulp-util');

Popover final #383

Are you sure you want to change the base?

Popover final #383

Conversation

krshubham commented Aug 30, 2017

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

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

atfornes commented Sep 1, 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

atfornes commented Sep 4, 2017

atfornes commented Sep 5, 2017

krshubham commented Sep 5, 2017 • edited Loading

krshubham commented Sep 5, 2017 •

edited

Loading