-
Notifications
You must be signed in to change notification settings - Fork 975
Conversation
wowzers this is starting to look good. cc @bradleyrichter |
Wow- great job, @cezaraugusto! 😄 |
yeah! exiting! Nice job @cezaraugusto ! |
looks like this needs a new rebase because it can't be merged in cleanly in github (it isn't ready to merge yet, but would be good to keep it up to date with master). Could you check for what the HTML meta tags we should be checking for are for the high res icons that you need? |
Let me know if you need any help with the grid layout. |
About high-res icons: I was thinking on getting images following Apple > MS conventions on links/meta icon declarations, then search for a given pattern for high res icons (maybe look for 1. Search for Apple name convention 2. Search for MS name convention (tile pictures) 3. Search for any name convention for high-res icons *4. Fallback for default .ico images |
@cezaraugusto This should work nicely. I expect half the ico files are 32x which may look clear enough. The other half at 16x are too small so we need to check for the dims and fall back from there to the letter/background theme. Doable? |
@bradleyrichter sure, sounds great |
@@ -802,7 +810,11 @@ class Frame extends ImmutableComponent { | |||
const isError = this.props.aboutDetails && this.props.aboutDetails.get('errorCode') | |||
if (!this.props.isPrivate && this.props.provisionalLocation === this.props.location && (protocol === 'http:' || protocol === 'https:') && !isError && savePage) { | |||
// Register the site for recent history for navigation bar | |||
appActions.addSite(siteUtil.getDetailFromFrame(this.frame)) | |||
const addSiteDebounced = debounce((frame) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were debouncing this so that we can call it multiple times. As the different events come in.
So you need to move addSiteDebounced
above at the top of the file. Then use it like you do below. But also use it in the other events as things come in like the favicon and the themeColor event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is sometimes we get a loadi-end but the favicon or themecolor event didn't happen yet. So you need to also call addSiteDebounced from:
this.webview.addEventListener('page-favicon-updated', (e) => {
this.webview.addEventListener('did-change-theme-color', ({themeColor}) => {
But be sure not to call it for private tabs, like the above one doesn't.
Great job, I have two things to say: |
1 - we're gradually improving new tab page, it will be connected with preferences so you can customize it your way. |
Thx a lot Cezar, If anyone has trouble finding the file here is the link in github And on a local windows install the file can be found at |
@cezaraugusto i built a local storage based cache for theme colors and favicons. if that could help you ping me. i did not make a pull request yet because i need to implement cache cleaning first... |
@lucidNTR thanks for your help! I'm not in this task yet but will definitely ping you if needed, thanks. |
@cezaraugusto I rebased against master and resolved all conflicts; I couldn't push to your remote since upstream editing wasn't allowed when this PR was created (you might be able to give me permissions by editing this PR) Here's the resolved branch though; I'll hit you up later via Slack about this 😄 |
Closing in favor of #5301 I'll be creating tasks based on what's left and referencing a lot of what's in here 😄 |
git rebase -i
to squash commits if needed.Still a lot of work to do. WIP of #3001
Current output: (will be updated as things gets done)
Tasks:
1.0 ENG tasks:
33k
build high-res favicon collector mechanism, pulled from page header at domain root page(not needed; best favicon URL is already fetched when page loads)create local cache for large favicons (base64)(not needed; favicons are already cached by browser)suggestions.js
ignore bookmarked sites on topSites grid about:newtab - Make suggestions.js ignore bookmarked sites on topSites grid #53081.0 Front-end tasks
Current Issues found (under investigation)