Closed Bug 1019841 Opened 10 years ago Closed 10 years ago

[Gallery] Update to use <gaia-header>

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(ux-b2g:2.1)

Status:
RESOLVED FIXED
Milestone:
2.1 S2 (15aug)
Project Flags:
ux-b2g 2.1

People

(Reporter: wilsonpage, Assigned: dmarcos)

References

Details

Attachments

(3 files, 1 obsolete file)

pull-request (master)
(deleted), text/x-github-pull-request
djf
: review-
Details
Pull Request (without bower)
(deleted), text/x-github-pull-request
djf
: review+
wilsonpage
: feedback+
Details
noBackgroundDeleteDialog.png
(deleted), image/png
Details 
      No description provided.
Attached file pull-request (master) (obsolete) (deleted) — 

  

  



    
    

    
  

      
      
      
      

        Attached file
          
        pull-request (master) (deleted)
        — 
      

    


  

  

  



        Attachment #8433552 -
        Attachment is obsolete: true

        Attachment #8464899 -
        Flags: review?(dflanagan)

    
    

    
  
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-

    
    

    
  

      
      
      
      

        Attached file
          
        Pull Request (without bower) (deleted)
        — 
      

    


  

  

  



        Attachment #8468954 -
        Flags: review?(dflanagan)

        Attachment #8468954 -
        Flags: feedback?(wilsonpage)

    
    

    
  
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)

    
  

        Attachment #8468954 -
        Flags: feedback?(wilsonpage)

    
    

    
  
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 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)

    
    

    
  
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.

  

  



    
  

        Attachment #8468954 -
        Flags: review?(dflanagan)

    
    

    
  
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.

  

  



    
  
Assignee: wilsonpage → dmarcos

    
  
Target Milestone: --- → 2.1 S2 (15aug)

    
    

    
  
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)

    
    

    
  
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 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-

    
    

    
  
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?

  

  



    
    

    
  
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+

    
    

    
  

      
      
      
      

        Attached image
          
        noBackgroundDeleteDialog.png (deleted)
        — 
      

    


  
Dialog with transparent background to illustrate problem described in comment #6

  

  



    
  

        Attachment #8468954 -
        Flags: review+ → review?(dflanagan)

    
    

    
  
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 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-

    
    

    
  
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)

    
    

    
  
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 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-

    
  
Blocks: 1053341

    
    

    
  
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)

    
    

    
  
Comment on attachment 8464899 [details]
pull-request (master)

Marked for review the wrong patch

  

  



        Attachment #8464899 -
        Flags: review?(dflanagan)

    
    

    
  
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 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+

    
    

    
  
Landed in master:

https://github.com/mozilla-b2g/gaia/commit/1e922d2e8f4f17242d029e14372ea43db412cd09

  

  


Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

    
    

    
  
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

  

  



    
    

    
  
(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?

  

  



    
    

    
  
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)

    
    

    
  
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 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+

        Attachment #8464899 -
        Flags: review+ → review-

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: