feature: New tab page #153
Conversation
newtab-page/newtab.html
Outdated
| <div id="newtab-vertical-margin"> | ||
| <div id="newtab-margin-top"></div> | ||
| <div> | ||
| <iframe src="https://duckduckgo.com/search.html?prefill=Search DuckDuckGo&focus=yes" style="display: block;overflow:hidden;margin: auto;padding:0;width:551px;height:40px;" frameborder="0"></iframe> |
There was a problem hiding this comment.
I feel like the & should probably be an & for historical reasons…
bundle.js
Outdated
| @@ -0,0 +1,2164 @@ | |||
| (function e(t,n,r){function s(o,u){if(!n[o]){if(!t[o]){var a=typeof require=="function"&&require;if(!u&&a)return a(o,!0);if(i)return i(o,!0);var f=new Error("Cannot find module '"+o+"'");throw f.code="MODULE_NOT_FOUND",f}var l=n[o]={exports:{}};t[o][0].call(l.exports,function(e){var n=t[o][1][e];return s(n?n:e)},l,l.exports,e,t,n,r)}return n[o].exports}var i=typeof require=="function"&&require;for(var o=0;o<r.length;o++)s(r[o]);return s})({1:[function(require,module,exports){ | |||
There was a problem hiding this comment.
Where did this come from? It seems kinda big…
There was a problem hiding this comment.
browserify created that
There was a problem hiding this comment.
I think I'd prefer we included something in a package.json file, and ran browserify as part of a build script.
| browser.topSites.get().then((topSitesArray) => { | ||
| var grid = document.querySelector("#newtab-grid"); | ||
| grid.innerHTML = ""; | ||
| for (let topSite of topSitesArray) { |
There was a problem hiding this comment.
So, on first run, I only see seven or eight top sites, and they're centered which looks kind of odd… Could we maybe use a grid layout to make things line up better? (I think grid-auto-columns and grid-auto-rows will help with this…)
scripts/newtab.js
Outdated
| grid.innerHTML += | ||
| "<div class='newtab-cell'><div class='newtab-site' draggable='true' type='history'>"+ | ||
| "<a class='newtab-link' title="+topSite.title+" href="+topSite.url+">"+ | ||
| "<span class='newtab-thumbnail thumbnail' style='background-size: cover; background-image: ;'></span>"+ |
There was a problem hiding this comment.
We should really also add a contextmenu item to let people copy the thumbnail url. (It was one of our most requested features!)
scripts/newtab.js
Outdated
| }, onError); | ||
| } | ||
| function initialize(){ | ||
| fetch(url).then(function(response){ |
There was a problem hiding this comment.
So, this is the second place we have code like this, if I remember correctly. Can we extract it into a utility function in another file?
scripts/newtab.js
Outdated
|
|
||
| var placeholders = []; | ||
| var url = 'https://bwinton.github.io/whimsy/thumbnail-gifs.txt'; | ||
| var getting = browser.storage.sync.get('newtab'); |
There was a problem hiding this comment.
Let's save this into a pref, like in the PR I submitted a couple of days ago…
scripts/newtab.js
Outdated
| //set default thumbnail | ||
| var href = link.href; | ||
| link.querySelectorAll(".thumbnail").forEach(function(img){ | ||
| //set gifs |
There was a problem hiding this comment.
This looks like a lot of code, let's pull it out into a function.
scripts/newtab.js
Outdated
| setThumbnail(); | ||
| //refresh open new tab pages | ||
| var querying = browser.tabs.query({title: "New Tab"}); | ||
| querying.then((tabs) => { |
There was a problem hiding this comment.
Maybe we should only reload the tabs if they're about:newtab (or whatever our moz-extension newtab url is)?
|
Some changes are taking too long, will come back to them at a later time. Issue #154 |
dnarcese
force-pushed
the
newtab-page
branch
2 times, most recently
from
da7cf72 to
7bde978
Compare
scripts/newtab.js
Outdated
| var tippyTopSites = require('tippy-top-sites'); | ||
| var pref = null; | ||
|
|
||
| // initialize('https://bwinton.github.io/whimsy/thumbnail-gifs.txt') |
There was a problem hiding this comment.
nit: remove commented code
scripts/newtab.js
Outdated
| } | ||
| }, onError) | ||
| .then(initialize('https://bwinton.github.io/whimsy/thumbnail-gifs.txt', setThumbnail)) | ||
| //.then(setThumbnail(placeholders)); |
No description provided.