Skip to content

Conversation

@marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Sep 9, 2017

This is what I have, so far. Feel free to add functionality / change the design, I will do the same :)

screenshot from 2017-09-09 15-23-16

@blizzz
Copy link
Member

blizzz commented Sep 11, 2017

cc @Espina2 @jancborchardt

@jancborchardt
Copy link
Member

Nice, good on the sidebar! Just few details:

  • The fav tags should be normal text size, check the mail app for example
  • The tag names not in italic - we never ever use italic
  • Best use flexbox in the content so the bookmarks expand to fit
  • Instead of the grid with gridlines, better use tiles? I guess @Espina2 has some mockup
  • Looking forward to some visuals like favicon or even screenshots ;)

@marcelklehr
Copy link
Member Author

  • heh, italic was kind of a desperate move, tbh, until I came up with the separator and the subheading :)
  • The add-bookmark-widget is probably unnecessarily prominent, moving it to the sidebar seems like a better solution to me.
  • Re tiles: I'm not sure I understand the difference between a grid and tiles. You mean using a background instead of borders would look better?
  • completely agree on flexbox

@marcelklehr
Copy link
Member Author

For screenshots, I'm aware of the following options (which are all not pretty):

  • use a server-side rendering mechanism. downside: require's installing additional software on the server
  • use a php proxy to route the bookmark url to our domain and then use http://html2canvas.hertzen.com/ to render the DOM in javascript and upload the image to our server. downside: you know... it's not pretty...
  • use a 3rd party service like https://developers.google.com/speed/pagespeed/insights/ that does the screenshot. upside: simple, downside: 3rd parties involved

so, for now I'd sadly opt for favicons and nice background colors for the tiles (if I understood that correctly :D)

@marcelklehr marcelklehr changed the title New ui New UI Sep 11, 2017
@jancborchardt
Copy link
Member

  • yup, adding bookmark could go in the sidebar, or as first tile of the main view :)
  • What I mean is that Grid: the current dividing lines. Tiles: Individual box for each of the elements, with whitespace. Howevee that's also not that cool.

Actually while we don't have visuals, I would do a list view with larger height rather than tiles, kind of like https://feedly.com does

@1ucay
Copy link

1ucay commented Sep 26, 2017

html2canvas is not a way. I have many issues with rendering, because many page is not html valid and framework renders strange thumbnails.

thumbnail would be awesome - what about https://github.com/microweber/screen

EDIT: I tried and works perfect with some settings. I can imagine proccess for creation of thumbnails. It can be trigger via cron and saved with needed maximum resolution to database blob field (like photo in contact app). We have option for render documents via openoffice on server side, so why not using phantomjs? :)

@marcelklehr
Copy link
Member Author

@1ucay Thumbnails would be really awesome, I agree. I'm a bit reluctant to require people to install additional software apart from the app, but if phantomjs can be bundled along with the app, then that would be a nice solution, imo.

@blizzz How do you feel about external dependencies / bundling phantomjs?

@jancborchardt
Copy link
Member

@marcelklehr as long as it can be bundled in a way which is available on a default PHP install that’s fine. Dependencies on other apps, or requirement to install additional packages is a no-no. :)

@Espina2
Copy link

Espina2 commented Sep 28, 2017

I can share what I did on the conference.

@marcelklehr
Copy link
Member Author

@Espina2 that would be awesome :)

@jancborchardt yea, that's what I thought.

@blizzz
Copy link
Member

blizzz commented Sep 30, 2017

@blizzz How do you feel about external dependencies / bundling phantomjs?

Last news about PhantomJS i know is, that the project is abandoned? → https://groups.google.com/forum/#!topic/phantomjs/9aI5d-LDuNE

It did not get any commit since June, too. For the record, I am against headless and any Chrome however. Wouldn't want to risk data leakage to the Ad company.

In general, external dependencies are OK, if reasonable – at least as long as it is PHP/JS, or given it can be pre-build and shipped out of the appstore. Some platform-specific dependencies or those that require other interpreters… this is too much of a hassle for the end user. The experience should be: click install → works.

For optional features we could say, OK, this needs package XYZ to be installed on your server. The basic functionality better works without it. For thumbnails it can be considered, if we Favicons instead, although this is already a lot less useful.

@blizzz
Copy link
Member

blizzz commented Sep 30, 2017

Of course there are not many options. For creating a thumbnail, we either need to have a web rendering engine in PHP (sounds like a bad idea already :)), use an API (data leakage) or rely on some installed web rendering engine or a wrapper, like PhantomJS (but we cannot rely on installed packages and cannot ship or build such).

We can do those three things

  1. Default to the favicons as most privacy protecting way of dealing with that
  2. Look for a free API and give an option to configure this (and offer to specify a compatible end point, for self-hosting – need to check if there's a popular project)
  3. Offer an option to use a locally installed headless browser like PhantomJS (or PhantomJS compatible, apparently there seem to be some alternatives that follow its API)

Security-wise it is safer to not run external code locally. Option 3 increases the attack surface. But in ghost of self-hosting, this is best.

My preference would be to go one way and do it right, unfortunately everything has a hook. Well, we can (and should) leave out 3, given that there is a proper way to self-host such a thumbnailer. The default favicon is not beautiful at all however, only I don't see away around with acceptable utilities, currently.

Another thought i just got: take the biggest picture on the page (if there is an) and combine it with the favicon (small, in a corner). It brings other problems though: avoid other domains (no spam, but CDNs are missed), need to fetch any image to figure out which is the biggest, JS and CSS would be ignored… OK, forget it :)

@jancborchardt
Copy link
Member

Well, the perfect is the enemy of the good so let’s take small steps, shall we? ;) The favicon (or rather higher res favicon-touch or apple-favicon-touch) would already be a great visual improvement on the current state. Then we can see in a separate pull request to improve on that.

@Espina2
Copy link

Espina2 commented Oct 8, 2017

I need a bit of extra-time already have some things to do. But I added this pull request to my fav list, so I will post something in the next few days.

@jancborchardt
Copy link
Member

Just stumbled across this website-to-pdf library which might work in generating the screenshots? https://github.com/alvarcarto/url-to-pdf-api

@blizzz
Copy link
Member

blizzz commented Oct 12, 2017

Just stumbled across this website-to-pdf library which might work in generating the screenshots? https://github.com/alvarcarto/url-to-pdf-api

requires Chrome…

@marcelklehr
Copy link
Member Author

https://github.com/mixu/electroshot might be a worthwhile option, since it doesn't use phantomJS but electron, which is much more up to date.

@blizzz
Copy link
Member

blizzz commented Dec 3, 2017

well, Electron again is an outdated Chrome/ium.

@marcelklehr
Copy link
Member Author

They do try to keep it up-to-date-ish, though, from what I've gathered. Then again, how would a nextcloud admin keep it up-to-date :/

@blizzz
Copy link
Member

blizzz commented Dec 3, 2017

If a browser, then a headless browser should be chosen over a GUI framework based on a browser. I guess there's no way around it… but what box of pandora it opens. Also this ideally would be sandboxed. The shipping nightmare alone… perhaps best is offering a 3rd party service to take this over (opt-in for privacy).

@jancborchardt
Copy link
Member

To repeat myself ;)

Well, the perfect is the enemy of the good so let’s take small steps, shall we? ;) The favicon (or rather higher res favicon-touch or apple-favicon-touch) would already be a great visual improvement on the current state. Then we can see in a separate pull request to improve on that.

Currently we want to do it all, and that prevents us from doing anything. :) So let’s start a bit smaller, with just the favicons. Ok?

@marcelklehr
Copy link
Member Author

I hear you :)

I'm focusing on making my companion browser extension useful atm, so that's why this pr has stalled ;)

@blizzz
Copy link
Member

blizzz commented Dec 12, 2017

Barely have time, but I started once to consolidate the JS… would be easier if the PR was on the project'S repo to contribute.

@jancborchardt
Copy link
Member

@marcelklehr I invited you to the @nextcloud organization so you should be able to make your branch branch off from the current bookmarks master so others can more easily contribute. :)

(@blizzz you can also add people to the org ;)

@blizzz
Copy link
Member

blizzz commented Dec 12, 2017

I thought I did long time ago already 😕

@marcelklehr
Copy link
Member Author

@blizzz hah, you did add me to the repo, but not to the organization :) I'm just too used to pushing to my fork, so my bad :)

@marcelklehr marcelklehr mentioned this pull request Jan 12, 2018
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants