Patryk Adamczyk [:patryk] UX Reporter
	Description • 10 years ago

At minimum it needs to have crisp graphics, currently the entire app is fuzzy. This is unacceptable, its a Mozilla app!

Ideally the app would get the visual refresh
+ the new header
+ flat icons
+ new app icon

	Dylan Oliver [:doliver]
	Comment 1 • 10 years ago

Since this is a Marketplace app rather than a Gaia app, it doesn't *technically* ship with any version. It is pre-installed in most retail builds so that might be splitting hairs a little bit, but it does mean that we also work to a different timeline with this app and nothing we do here would be considered blocking 2.0.

We would welcome new designs and will make a best effort to get them completed prior to 2.0 retail launch.

	Peter La
	Comment 2 • 10 years ago

Thanks Patryk.  I'll upload a new icon this week.  Stay tuned.

	Peter La
	Comment 3 • 10 years ago

needinfo'ing myself as a reminder.

	Stephany Wilkes
	Comment 4 • 10 years ago

Dylan, when Peter is done with the graphics work, who could help us to package and ship the update? Thanks!

	Dylan Oliver [:doliver]
	Comment 5 • 10 years ago

(In reply to Stephany Wilkes from comment #4)
> Dylan, when Peter is done with the graphics work, who could help us to
> package and ship the update? Thanks!

I can package & ship.

	Peter La
	Comment 6 • 10 years ago

Great, thanks Dylan.  We're aiming to ship the assets by EOD Monday/early Tuesday next week.

	Peter La
	Comment 7 • 10 years ago

Dylan,

I'll need you to provide:

1) a list of all the graphics used in this app
2) a zip file containing all the currently used assets so I can swap them out with the new ones

for #2, keep in mind that I will have to provide 5 sizes for every asset so that this app looks good on every screen size (@1, @1.5, @2, @2.25, @3.375).

	Dylan Oliver [:doliver]
	Comment 8 • 10 years ago

Hi Peter,

You can find the graphics in the github repo: 
https://github.com/mozilla-b2g/notes/tree/master/images

As far as I can tell, just about everything in there is actively used by the app. I'm not able to see the "attachment.png" graphic visible in the app anywhere but there's still a code reference to it so better safe than sorry on that one, I guess.

	Peter La
	Comment 9 • 10 years ago

Hi Dylan,

Thanks for this.  So I noticed that there are no header images in this repo.  The Notes app is still currently using the old orange headers with gradients.  How do you want to update the headers?  Can it use the building blocks?

Also, some icons are saved out in strange sizes that are non-square.  I'm going to correct this and save them out at the sizes we normally do for action icons, but it will likely require you to make position adjustments within the code to make sure the icons don't shift compared to where they are now.

Let me know what you think,

Peter.

	Peter La
	Comment 10 • 10 years ago

Hi Dylan,  please see my comment above and also let me know if it's ok for me to correct the icon sizes or if you just want me to ship them in the sizes/dimensions exactly as they are in github right now - but just add all the other sizes so that they are crisp at all resolutions.

	Dylan Oliver [:doliver]
	Comment 11 • 10 years ago

Unfortunately we don't have any developers actively working on the Notes app in the 2.0/2.1 time frame so for now my preference would be to maintain the existing sizes so we can swap them in. That way we can be certain to get them published relatively quickly.

	Peter La
	Comment 12 • 10 years ago

New app icon for Note @1x, @1.5x, @2x, @2.25x, @3.375x and @512x512px.

The 512x512px version is the new recommended size for Marketplace app submissions going forward (128x128px is still the required).

	Peter La
	Comment 13 • 10 years ago

Here are the major icons sets (separated into header and non-header icons) @ all 5 sizes.

Note:
I took some liberties with 3 of the icons in terms of their dimensions and I'm hoping it won't affect things if the code just has them center aligned.  These are listed below.

1) empty_list.png is now 210x210
2) empty_trash.png is now 210x210
3) attachment.png is now 16x16

We are planning to contact Pavel Ivanov to work on flattening out the headers  and removing the header drop shadows (I believe there are 2 headers - orange and green).  Will provide bitmap assets for headers if needed at that point.

	Peter La
	Comment 14 • 10 years ago

All new Notes source PSD files can be found here:
https://mozilla.box.com/s/a6jb26nus4t3a357v9hy

	Peter La
	Comment 15 • 10 years ago

Ping - Dylan.  See Comments 12 - 13.  Can either you or Pavel put the new graphics into the app?

Pinging Pavel about flattening out the headers.  Pavel, please speak to Patryk for more details about this, and let him know if you need graphics help for the headers.

Adding patryk to the needinfos for awareness.

Please flag me or Patryk (or both) for ui-review.

	Dylan Oliver [:doliver]
	Comment 16 • 10 years ago

Here's a pull request for the application icons -- flagging Pavel for review.

Will need more time for the in-app images unless Pavel is able to get to them first.

	Dylan Oliver [:doliver]
	Comment 17 • 10 years ago

Peter - we're about to publish a Notes update to add some new locales but I won't have time to get to the rest of the images before then. Would you prefer me to hold off on landing/publishing the new app icon until we're able to get all of these done?

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 18 • 10 years ago

Comment on attachment 8463668 [details]
PR for new application icons

LGTM :)

BTW: I can start working tomorrow on this one (I will start with implementing current headers) can I ask you for r? when I'm ready?

	Dylan Oliver [:doliver]
	Comment 19 • 10 years ago

Thanks Pavel, that's great. You can start with me and if I get in over my head I'll redirect it.

	Dylan Oliver [:doliver]
	Comment 22 • 10 years ago

Hi Pavel,

The new headers are great, it's a nice change. I'm going to ask Yan to do the full review, but from playing with it, here are a couple of things I've found:

* The font color in the header of the Settings page is wrong. You'll need to connect to an Evernote account and then click on the gear icon in the folder list to access it. It's using a grey color which is hard to read on the green background instead of the white in other pages

* If you are editing a note and touch the note title to change it, the font switches to the old bold & blocky font instead of fira, and then changes back when you are done editing. You get the same thing if you edit the name of the Notebook when you're in the list view, and in this case it overruns the drawer icon a little bit.
** we also have another bug in this area (bug 995527) while you're in the neighborhood in case it's a simple fix :)

* Should the "Notebooks" title in the drawer header use fira italic to match the other headers?

	Dylan Oliver [:doliver]
	Comment 25 • 10 years ago

Screenshots for the items in comment #22.

	Dylan Oliver [:doliver]
	Comment 26 • 10 years ago

Comment on attachment 8464301 [details]
PR - sync gaia/shared with app/shared

Yan, can you take the review for Pavel's patch to update the Notes headers & graphics?

	Dylan Oliver [:doliver]
	Comment 27 • 10 years ago

Cleaning off the ni? for Peter from comment #17 - we won't publish the app icons until we land Pavel's work here too.

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 28 • 10 years ago

Hey Dylan,

I know these issues ... just want to go with small steps because of the review process small patches are easy for review :)

and we should agree how we will proceed with these changes 1 Big PR or few small?

	Dylan Oliver [:doliver]
	Comment 29 • 10 years ago

(In reply to Pavel Ivanov [:ivanovpavel] from comment #28)
> I know these issues ... just want to go with small steps because of the
> review process small patches are easy for review :)

Got it, no problem. 

> and we should agree how we will proceed with these changes 1 Big PR or few
> small?

Smaller PRs are fine, though we usually do just on PR per bug so we might need to open some followup bugs for the other issues.

	Yan Or
	Comment 30 • 10 years ago

Comment on attachment 8464301 [details]
PR - sync gaia/shared with app/shared

Looks good.  One thing is that the 2.0 header includes logic to resize and center title text.  This is done using shared/js/font_size_utils.js.  Perhaps we should also include that.

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 31 • 10 years ago

Yep, Thanks Yan :)

	Dylan Oliver [:doliver]
	Comment 32 • 10 years ago

Comment on attachment 8463668 [details]
PR for new application icons

Obsoleting app icon patch and moving to Bug 1047566.

	Dylan Oliver [:doliver]
	Comment 33 • 10 years ago

Pavel, when you're ready to land this one, can you add 'checkin-needed' to the keywords field?

	Dylan Oliver [:doliver]
	Comment 34 • 10 years ago

(In reply to Dylan Oliver [:doliver] from comment #33)
> Pavel, when you're ready to land this one, can you add 'checkin-needed' to
> the keywords field?

Oops, nevermind, it looks like you have merge permissions here. Land when ready!

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 35 • 10 years ago

Sure :) thanks :) 

I will fix few more things today and I will land it and I will go forward :)

	Peter La
	Comment 36 • 10 years ago

(In reply to Dylan Oliver [:doliver] from comment #27)
> Cleaning off the ni? for Peter from comment #17 - we won't publish the app
> icons until we land Pavel's work here too.

Thanks Dylan.  Sorry for the late reply here - I agree with the decision.

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 37 • 10 years ago

Hey Dylan,

can you help me with font_size_utils.js we need to fire a `lazyload` event but I'm not sure where? and after that my PR is ready for r+ again

	Dylan Oliver [:doliver]
	Comment 38 • 10 years ago

Yan, can you help Pavel?

	Yan Or
	Comment 39 • 10 years ago

gmarty or mhenretty are the right owners to ask about this.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 40 • 10 years ago

We need to fire a `lazyload` custom event anywhere in the notes app where we add a header building block (ie. <header><h1>{stuff}</h1></header>). This ensures the centering/resizing logic will run whenever we modify content there (through a MutationObserver). Unfortunately, I'm not familiar enough with the notes app to know all the places where this happens.

	Dylan Oliver [:doliver]
	Comment 41 • 10 years ago

Pavel, are you able to add the changes mhenretty specified in comment #40? We got stalled on this PR but I'd love to get it finished up and published.

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 42 • 10 years ago

Hey Dylan,

I tried but it doesn't work ... maybe I miss something. Can you help?

	Dylan Oliver [:doliver]
	Comment 43 • 10 years ago

mhenretty, can you point us to an example of the lazyload event in another app?

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 44 • 10 years ago

Sure, you can just grep for 'lazyload' in gaia/apps for more examples. Here's one:

https://github.com/mozilla-b2g/gaia/blob/6e3fd39017cf279a423b9df09889ac971503b5af/apps/bookmark/js/bookmark_editor.js#L49

	Dylan Oliver [:doliver]
	Comment 45 • 10 years ago

Pavel, what is the error you are seeing that exposes the lack of the lazyload event?

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 46 • 10 years ago

no error ... can you check this one https://github.com/mozilla-b2g/notes/pull/47/files#diff-fde4e313d5f4400cd7405ed9baa7053cR2148

not sure if this is the right way

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 47 • 10 years ago

(In reply to Pavel Ivanov [:ivanovpavel] from comment #46)
> no error ... can you check this one
> https://github.com/mozilla-b2g/notes/pull/47/files#diff-
> fde4e313d5f4400cd7405ed9baa7053cR2148
> 
> not sure if this is the right way

I can weigh in here. You are only dispatching this event once when the entire common.js file is loaded. Instead what you need to do is dispatch that event _after_ you add any new headers to the DOM. Also, it's a little less efficient to use the entire `document.body` rather then the new header element itself.

	Dylan Oliver [:doliver]
	Comment 48 • 10 years ago

Hi Pavel, I had good results by sending the event as the last line before the return in Cards.goTo -- https://gist.github.com/dylano/97db6f080c0aed5041ca

I found a simple test case was to start up a clean copy of the app and access the "Trash" folder. Attaching a screen grab here with before and after results. 

If you have a linked Evernote account, the Settings page also clearly shows the off-center behavior and was fixed with the above event placement.

	Dylan Oliver [:doliver]
	Comment 49 • 10 years ago

Yan, can I get your feedback on this change:
https://gist.github.com/dylano/97db6f080c0aed5041ca#file-cards-js-L44-L49

Based on mhenretty's comment #47, does this look like an appropriate place to make the change?

	Yan Or
	Comment 50 • 10 years ago

From mhenretty's comments and my understanding of the library, seems like we only need to fire this off once after index.html is loaded (all header nodes are included there and we don't inject any more into the DOM dynamically).

Sorry, I don't have time look into this further.

	Dylan Oliver [:doliver]
	Comment 51 • 10 years ago

Based on Yan's feedback I moved the block into initCards() instead so it only gets fired once and it still works with the instances I identified. Pavel, can you include this with your patch and then I think we're ready to land? 

https://gist.github.com/dylano/74b7966b5d8774788ce8#file-cards-js-L25-L30

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 52 • 10 years ago

Hey Dylan,

thanks for help :)
PR is updated, can you check it one more time before merge?

	Dylan Oliver [:doliver]
	Comment 53 • 10 years ago

Checked on device -- looks good to me. Thanks!

	Pavel Ivanov [:ivanovpavel][:pivanov] UX Assignee
	Comment 54 • 10 years ago

Landed:
https://github.com/mozilla-b2g/notes/commit/842e8dc0887a67681335e02151c63b89f577f01a