Popover final #383
Popover final #383
Conversation
publish new email images
| let btn = event.target; | ||
| //cannot inject the spinner HTML directly here | ||
| let inHTML = ` | ||
| <style> |
There was a problem hiding this comment.
Style should go to a sass file, thanks!
| margin: 150px 0; | ||
| } | ||
| </style> | ||
| <div class="pos-r"> |
There was a problem hiding this comment.
Html should be written in a template file :)
| let clientRect = range.node.nextSibling ? | ||
| range.node.nextSibling.getBoundingClientRect() : | ||
| range.node.parentElement.getBoundingClientRect(); | ||
| div.innerHTML = inHTML; |
There was a problem hiding this comment.
I moved almost all the styles to sass only what seemed as necessary to me is kept here
| </svg> | ||
| </div> | ||
| </div>`; | ||
| <div class="pos-r"> |
| else { | ||
| div.style.height = '110px'; | ||
| innerEle.innerHTML = document.getElementById('no-title-in-url').innerHTML; | ||
| innerEle.querySelector('#popoverLinkTitle').innerHTML = urlTitle; |
There was a problem hiding this comment.
Also, why not use angular to populate the title, description, etc. with {{popover.title}} and similar?
There was a problem hiding this comment.
Oh yes, I'll surely do that! Poor me!
There was a problem hiding this comment.
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!
|
|
||
|
|
||
| function linkPreview($http) { | ||
| const LINK_PREVIEW_SERVER_URL = 'http://localhost:9090/fetch'; |
There was a problem hiding this comment.
Server URL to config file
There was a problem hiding this comment.
Sure, i'll update the code
There was a problem hiding this comment.
I need your help here ! @atfornes ! What would be the best way to do this?
| @@ -66,3 +66,41 @@ | |||
| </div> | |||
|
|
|||
| </div> | |||
|
|
|||
| <div style="display: none;"> | |||
There was a problem hiding this comment.
Style in sass files is preferred over inline style. Thanks!
src/sass/pad.sass
Outdated
| @@ -227,7 +227,7 @@ div | |||
| position: absolute | |||
| border: 1px solid $grey | |||
| z-index: 3 | |||
| background-color: lightgrey | |||
| background-color: darken(#fff, 5%) | |||
There was a problem hiding this comment.
why not to use one of our colors as $light-grey?
There was a problem hiding this comment.
Its a bit more darker than what I wanted
There was a problem hiding this comment.
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?
|
Sorry, I see now that some of the comments I did were not yet sent |
gulpfile.js
Outdated
| @@ -220,7 +220,16 @@ var gulp = require('gulp'), | |||
| spawn = require('child_process').spawn, | |||
| gutil = require('gulp-util'); | |||
|
|
|||
There was a problem hiding this comment.
changes in gulpfile should go to the pull request about error handling in gulp
There was a problem hiding this comment.
I'll be fixing this soon! Sorry about this, I didn't check that I had updated gulpfile here
| <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"> |
There was a problem hiding this comment.
please, remove inline css and bring it to style files
There was a problem hiding this comment.
Sorry :( ! I'll push the changes soon.
|
@krshubham, this pull request still have unattended comments, please answer them. When can we expect the updated pull request? |
|
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. |
|
Okay, I'll open an issue listing all the work that should be done ! 👍 |
In this PR, respective html and css for the popover were added in the corresponding files.
Future work: