Closed
Bug 1019841
Opened 10 years ago
Closed 10 years ago
[Gallery] Update to use <gaia-header>
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(ux-b2g:2.1)
People
(Reporter: wilsonpage, Assigned: dmarcos)
References
Details
Attachments
(3 files, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Attachment #8433552 -
Attachment is obsolete: true
Attachment #8464899 -
Flags: review?(dflanagan)
Comment 3•10 years ago
|
||
Comment on attachment 8464899 [details]
pull-request (master)
Could you prepare a simpler version of the patch that uses shared/elements/ instead of bower? I suspect that we will eventually adopt bower for the gallery app, but I'm not ready to make that decision yet.
Also, I'm concerned about the performance implications of depending on an icon font that includes all the icons for all of gaia. How does this affect:
1) the build time size of the application.zip file that gets pushed on the phone? (I'm guessing that the switch from png to svg will make this go down)
2) the run time memory usage of the app?
3) the startup time of the app?
I'm guessing that this has been discussed and debated elsewhere, and if so, you can just point me to that discussion. And I don't need gallery-specific measurements. If you've measured the impact of the icon font on some other app, that should be enough to reassure me.
Do I understand correctly that gaia-header requires gaia-icons and that gaia-icons includes most of the the glyphs for most of the apps? Is there some way that we could customize this on a per-app basis?
Attachment #8464899 -
Flags: review?(dflanagan) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8468954 -
Flags: review?(dflanagan)
Attachment #8468954 -
Flags: feedback?(wilsonpage)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
- Needs rebasing against master then all icon glasses need changing from class="icon-foo" to data-icon="foo".
- Need to add missing gaia-icons stylesheet in the <head>
Attachment #8468954 -
Flags: feedback?(wilsonpage)
Assignee | ||
Updated•10 years ago
|
Attachment #8468954 -
Flags: feedback?(wilsonpage)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
Looks good, last things:
1. Add <link> to gaia-icons in index.html
2. Background to delete overlay is missing, not sure why. Should look the same as share overlay.
Attachment #8468954 -
Flags: feedback?(wilsonpage) → feedback+
Comment 7•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
Thanks for working on this Diego. I've left a couple of comments on github, but am clearing the review request until you address the feedback that Wilson left. Just set r? again when that is fixed.
Attachment #8468954 -
Flags: review?(dflanagan)
Assignee | ||
Comment 8•10 years ago
|
||
I have addressed all Wilson's comments. The only remaining issue was the missing background in the delete overlay. The problem does not reproduce in wilson's patch because it's not rebased against latest master. shared/style/confirm.css has recently (bug 1039631) removed the background images. The background in gallery.css (#confirm-dialog) applies instead. This is an example of a change in a shared component that affects an application without us being aware of it. It illustrates why it's important that each app can manage its own dependencies and decide when to update.
Assignee | ||
Updated•10 years ago
|
Attachment #8468954 -
Flags: review?(dflanagan)
Assignee | ||
Comment 9•10 years ago
|
||
In the updated PR I removed the styles for #overlay and #confirm-dialog in gallery.css. We should shared/style/confirm.css take care of it.
Reporter | ||
Updated•10 years ago
|
Assignee: wilsonpage → dmarcos
Updated•10 years ago
|
Target Milestone: --- → 2.1 S2 (15aug)
Comment 10•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
Clearing the review request because it looks like you forgot to push the new commit to github.
Reset the flag when the PR is updated.
Attachment #8468954 -
Flags: review?(dflanagan)
Flags: needinfo?(dmarcos)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8468954 [details] Pull Request (without bower) I updated the PR with a style removal I think it's not necessary anymore. shared/confirm.css makes it redundant. https://github.com/mozilla-b2g/gaia/pull/22603/files#diff-7799e388381b4a1abd55fcf1b815f99dL72
Attachment #8468954 -
Flags: review?(dflanagan)
Flags: needinfo?(dmarcos)
Comment 12•10 years ago
|
||
Comment on attachment 8468954 [details] Pull Request (without bower) r- because there are comments on github that I left a two days ago that you have not addressed. (At least one is one that I originally left on Wilson's original bower-based PR as well.) Also, I can't understand how removing style declarations can fix an issue where the delete confirmation dialog did not have a background. I'm misunderstanding what Wilson was reporting in comment 6 or misunderstanding your explanation in comment 8. What was the missing background issue that Wilson was reporting, and how does your second commit, removing styles but not adding any, fix that?
Attachment #8468954 -
Flags: review?(dflanagan) → review-
Assignee | ||
Comment 13•10 years ago
|
||
The delete dialog did have a background applied from gallery.css but it was almost transparent. You can checkout the commit before the one that removes the style and reproduce the problem by going to gallery and deleting a photo (screenshot attached): git checkout 9a88f52 The style I removed (black with 0.4 opacity) from gallery.css in the patch was overriding the one in share/style/confirm.css. We should let share/style/confirm.css style our dialogs for consistency across gaia. gallery.css #overlay, #confirm-dialog { background-color: rgba(0, 0, 0, 0.4); } confirm.css form[role="dialog"][data-type="confirm"] { background: #2d2d2d; } (In reply to David Flanagan [:djf] from comment #12) > Comment on attachment 8468954 [details] > Pull Request (without bower) > > r- because there are comments on github that I left a two days ago that you > have not addressed. (At least one is one that I originally left on Wilson's > original bower-based PR as well.) > > Also, I can't understand how removing style declarations can fix an issue > where the delete confirmation dialog did not have a background. I'm > misunderstanding what Wilson was reporting in comment 6 or misunderstanding > your explanation in comment 8. What was the missing background issue that > Wilson was reporting, and how does your second commit, removing styles but > not adding any, fix that?
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
Sorry for missing your GH comments. I updated the PR to address them
Attachment #8468954 -
Flags: review- → review+
Assignee | ||
Comment 15•10 years ago
|
||
Dialog with transparent background to illustrate problem described in comment #6
Assignee | ||
Updated•10 years ago
|
Attachment #8468954 -
Flags: review+ → review?(dflanagan)
Comment 16•10 years ago
|
||
Ah, I understand now. The shared file had a background-image that was being displayed, but when that was removed we got the background-color from gallery.css instead of the shared color. I'll give this patch a final review today.
Comment 17•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
r- because the change to open.html breaks open.js which still expects and element with id "menu". STR: use the SMS app and attach a photo to a message. The pick activity works fine. Now tap on the thumbnail in the SMS app and pick "view". See that the image is not displayed and that there is a JS error in the console. I think just changing the id from "menu" to "save" in open.js will probably be enough to fix this. Though you should check somehow that the save button is hidden correctly when it is not needed.
Attachment #8468954 -
Flags: review?(dflanagan) → review-
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
Changed selector in open.js to match the new DOM
Attachment #8468954 -
Flags: review- → review?(dflanagan)
Comment 19•10 years ago
|
||
Marking this bug as required for 2.1 since the web components Header is the one committed web component for 2.1. Other web components for 2.2 depend on Header going first. Sorry for doing this late; Hema just brought this bug to my attention today, but it reflects agreed scope and is in the agreed 2.1 plan.
ux-b2g: --- → 2.1
Comment 20•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
I was wrong about only needing a change at line 61. The old id "menu" is still in use in at least two other places in open.js
With the current patch the Save button starts out hidden and is never displayed. If you receive an image in an MMS message, tap on it and select View, you ought to see the Save button in the titlebar, I think.
Attachment #8468954 -
Flags: review?(dflanagan) → review-
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8464899 [details]
pull-request (master)
Updated PR to show/hide the save button on the activity header
Attachment #8464899 -
Flags: review- → review?(dflanagan)
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8464899 [details]
pull-request (master)
Marked for review the wrong patch
Attachment #8464899 -
Flags: review?(dflanagan)
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
Changed PR to hide/show save button on activity header
Attachment #8468954 -
Flags: review- → review?(dflanagan)
Comment 24•10 years ago
|
||
Comment on attachment 8464899 [details] pull-request (master) Diego, It took me a while to figure out how to test the Save button. But I've done that now and it looks good. Just two nits I'd suggest you fix before landing: The first call to hideSaveButton() is not needed because the button starts out hidden. The code to work around bug 870619 does not seem to be needed with the new header, so I'd suggest you remove that from showSaveButton() and hideSaveButton().
Attachment #8464899 -
Flags: review+
Assignee | ||
Comment 25•10 years ago
|
||
Landed in master: https://github.com/mozilla-b2g/gaia/commit/1e922d2e8f4f17242d029e14372ea43db412cd09
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 26•10 years ago
|
||
You guys are killing me with the !important usage =( Landed a follow-up to fix the xfail, we really need to get this working on TBPL: https://github.com/mozilla-b2g/gaia/commit/d92ebda55c9b203ee21ad5db3bf48a2435da6e6e
Reporter | ||
Comment 27•10 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #26) > You guys are killing me with the !important usage =( We have no alternative right now do we?
Assignee | ||
Comment 28•10 years ago
|
||
The !important is IMPORTANT. It's a work around for one of the current limitations of web components. As far as understand styles inside the component applied to the projected content are more specific than those applied from the outside. If there's an alternative I'm happy to change the approach. I should have added a comment to the !important
Flags: needinfo?(kgrandon)
Comment 29•10 years ago
|
||
Since we've already landed several apps with !important so far, let's keep doing that - but I'm sure there are plenty of alternatives. I don't know whether these are better or worse than important :) In the meantime, tracking new usage of !important with a bug # would be good. They can all point to the same bug #. Thanks!
Flags: needinfo?(kgrandon)
Comment 30•10 years ago
|
||
Comment on attachment 8468954 [details]
Pull Request (without bower)
I set my r+ on the wrong version of the patch. Fixing that now.
Attachment #8468954 -
Flags: review?(dflanagan) → review+
Updated•10 years ago
|
Attachment #8464899 -
Flags: review+ → review-
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•