Closed Bug 1011601 Opened 10 years ago Closed 10 years ago

[Dialer] Update to use <gaia-header>

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

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

Tracking

(ux-b2g:2.1)

Status:
RESOLVED FIXED
Project Flags:
ux-b2g 2.1

People

(Reporter: yor, Assigned: wilsonpage)

References

Details

Attachments

(1 file, 4 obsolete files)

pull-request (master)
(deleted), text/x-github-pull-request
wilsonpage
: review+
Details 
      No description provided.
Attached file WIP (obsolete) (deleted) — 

  

  



        Attachment #8425211 -
        Flags: feedback?(wilsonpage)

        Attachment #8425211 -
        Flags: feedback?(kgrandon)

        Attachment #8425211 -
        Flags: feedback?(arnau)

        Attachment #8425211 -
        Flags: feedback?(21)

    
  
Assignee: nobody → yor
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8425211 [details]
WIP

New markup looks good to me. Which platform fixes were you waiting for here specifically?

  

  



        Attachment #8425211 -
        Flags: feedback?(kgrandon) → feedback+

    
    

    
  
(In reply to Kevin Grandon :kgrandon from comment #2)
> Comment on attachment 8425211 [details]
> WIP
> 
> New markup looks good to me. Which platform fixes were you waiting for here
> specifically?

Bug 1003294, which pretty much blocks all web component related work.
Bug 1007743 also blocks l10n testing for all of these.

  

  



    
    

    
  
Good to know. I was seeing bug 1003294 manifest in the browser, but not on an actual device. Have you noticed it on a device yet? If so that would be bad as we landed the gaia-switch component in FTU today.

  

  



    
    

    
  
Yes, I have seen it on my peak phone with gaia-header in the call log.

  

  



    
    

    
  
(In reply to Kevin Grandon :kgrandon from comment #4)
> Good to know. I was seeing bug 1003294 manifest in the browser, but not on
> an actual device. Have you noticed it on a device yet? If so that would be
> bad as we landed the gaia-switch component in FTU today.

I've seen it manifest only under certain conditions. One of which is rendering content after initial page load.

  

  



    
    

    
  
Comment on attachment 8425211 [details]
WIP

Saw the @import bug rearing its ugly head which was breaking the call-log header appearance. Aside from that, looks on course :)

  

  



        Attachment #8425211 -
        Flags: feedback?(wilsonpage) → feedback+

    
    

    
  
Comment on attachment 8425211 [details]
WIP

Let's move the feeback over to Anthony.

Anthony please note that there is a lot of things that does not makes us happy with Web Components right now. Lack of platform support force some workarounds/hacks.

Those will be addressed in a second phase once the platform issues are fixed and at the end you will end up with a shiny component.

  

  



        Attachment #8425211 -
        Flags: feedback?(21) → feedback?(anthony)

    
  

        Attachment #8425211 -
        Attachment is obsolete: true

        Attachment #8425211 -
        Flags: feedback?(arnau)

        Attachment #8425211 -
        Flags: feedback?(anthony)

    
    

    
  

      
      
      
      

        Attached file
          
        Pull Request (obsolete) (deleted)
        — 
      

    


  

  

  



        Attachment #8429310 -
        Flags: review?(wilsonpage)

        Attachment #8429310 -
        Flags: review?(anthony)

        Attachment #8429310 -
        Flags: review?(21)

    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

Same thing for the review. It looks good to me but let's delegate to Rik, this is sufficient.

  

  



        Attachment #8429310 -
        Flags: review?(21)

    
  
Status: ASSIGNED → NEW

    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

Yan, looks good to me :)

Going to defer to Rik for final r+ in this.

  

  



        Attachment #8429310 -
        Flags: review?(wilsonpage)

    
    

    
  
Hey guys,
I've created a patch to be applied on top of Yan's PR to include gaia-buttons instead of using the edit mode BB: https://github.com/rnowm/gaia/commit/63cd2bd9e6cbd23ddfe400f0497d5f95c1ff55df.patch

Rik,
should we included this patch in Yan's PR or make more sense doing a follow up bug?

  

  



    
  
Status: NEW → ASSIGNED

    
    

    
  
Arnau: Follow up please.

  

  



    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

This is not part of this bug but let me mention it:
data-skin="organic" feels weird to me. It should be a class. Maybe it's a Web Component implementation restriction, but if it's not, please consider having more sensible attributes for our web components.

  

  



        Attachment #8429310 -
        Flags: review?(anthony) → review+

    
    

    
  
Hey Yan,
please wait till 2.1 to land this, because of the header centering.
Thanks!

  

  



    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

Anthony,

Rebased off latest master.  Please review.  Thanks.

  

  



        Attachment #8429310 -
        Flags: review+ → review?(anthony)

    
    

    
  
Marking this bug 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. Just updating the flags appropriately for release tracking.

  

  


ux-b2g: --- → 2.1

    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

Taking this for now as requested by email. I'll feedback? Anthony when I think it's r+.

I left some comments in the PR. Please verify that the following are ok:
* call_log.js and suggestions_list.js are creating/using header elements still. I think this is ok, but please verify that removing the headers.css file didn't regress this.
* elements/add-contact-action-menu.html still has a header element. This seems to be part of an action menu, so I think this is ok.
* elements/suggestion-overlay.html still has a header element. This seems to be part of a gaia-menu, so I think this is ok.

  

  



        Attachment #8429310 -
        Flags: review?(anthony) → review-

    
    

    
  
Confirmed that call_log.js and suggestions_list.js use of headers are fine.

  

  



    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

Updated patch based on feedback.

  

  



        Attachment #8429310 -
        Flags: review- → review?(drs+bugzilla)

    
    

    
  
Comment on attachment 8429310 [details]
Pull Request

See comments on PR.

  

  



        Attachment #8429310 -
        Flags: review?(drs+bugzilla) → review-

    
    

    
  

      
      
      
      

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

    


  
UPDATED

- Fixed incorrect disabled button style

  

  



        Attachment #8429310 -
        Attachment is obsolete: true

        Attachment #8476645 -
        Flags: review?(drs+bugzilla)

    
    

    
  
Doug,

Wilson's PR has the same patch as mine but rebased off the latest master that has the disabled support.
I tested it and looks fine.  Can you give a quick check?

Thanks.

  

  



    
    

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

I don't like how the "delete" button fades in and out when you select/deselect an item in the call log edit mode. Yan and I talked on IRC and he's going to talk with Wilson about it. Our options, as far as I can tell here, are:

1) We remove the transition from WC.
2) We override disable it in the dialer.
3) We let it go.

I personally think we should remove this transition entirely (#1) as it will look like crap on the hardware that we're targeting and will make it harder to mass-deploy WC.

Since I took this review from Anthony without explicit permission, I'm feedbacking him on it, since this is effectively done.

  

  



        Attachment #8476645 -
        Flags: review?(drs+bugzilla)

        Attachment #8476645 -
        Flags: review+

        Attachment #8476645 -
        Flags: feedback?(anthony)
Flags: needinfo?(yor)
Flags: needinfo?(wilsonpage)

    
    

    
  
Carol and Przemek (whomever can get to this first depending on time zone), please advise as to whether this header transition is required and/or desired. I see the advantages with removing it. Thanks!

  

  


Flags: needinfo?(pabratowski)
Flags: needinfo?(chuang)

    
    

    
  
Hi Doug, 
I'm not sure if it's related to the header change for the 'delete' transition.
But in my opinion, I would suggest not to use fade in/ out transition. The delete button could just switch to another color when select/deselect an item. It's clear enough without having a transition.

I'll need info Przemek for his comment because it's related to Building blocks design.
Thank you!

  

  


Flags: needinfo?(chuang)

    
    

    
  
As discussed on IRC. This is currently an issue specific to the gaia-header component, not Dialer and should not block this patch from landing. The property transitions of buttons within gaia-header will tracked as part of bug 1057005.

Let's continue discussion over there :)

  

  


Flags: needinfo?(wilsonpage)
See Also:  → https://bugzilla.mozilla.org/show_bug.cgi?id=1057005

    
    

    
  
Just talked to Wilson about this.

  

  


Flags: needinfo?(pabratowski)

    
    

    
  
The patch is green and scheduled to land today.

  

  



    
    

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

f- based on the comments I left on Github.

I've also seen that there is an accessibility issue that needs to be fixed before landing this.

  

  



        Attachment #8476645 -
        Flags: feedback?(anthony) → feedback-
Depends on: 887541

    
    

    
  
I'd like to understand what the ux-b2g: 2.1 flag is. This will be helpful to know in order to prioritize reviews.
Also, is there any user facing improvement with moving to gaia-header?

  

  



    
    

    
  
Sorry - I sent an email about that to several lists a while ago. I'll see if I can find it. No one else but UX and release really needs to worry about it. It's a way for us to mark bugs that need some work from someone on the UX team during a certain release, and that we expect to ship.

It's not necessarily about what the user can see. In this case, web components are tied in to 2.2 styles work. They are the "how" behind our "what." Both Przemek and Casey, from the UX team, have been working on aspects of web components.

  

  



    
    

    
  

      
      
      
      

        Attached file
          
        Updated PR (obsolete) (deleted)
        — 
      

    


  

  

  


Flags: needinfo?(yor)

    
  

        Attachment #8477646 -
        Flags: review+

    
  

        Attachment #8476645 -
        Attachment is obsolete: true

    
    

    
  
Updated PR based on Anthony's feedback and carrying the r+ over.  Ready to land.

  

  



    
    

    
  
Comment on attachment 8477646 [details]
Updated PR

This needs feedback+ from Anthony before it's ready to land. I'm also not happy with the discussion (or lack thereof) in bug 1057005. I want one of the following to be done first:

1) We add a selector which disables the transition for just the dialer, or
2) A decision is reached in bug 1057005 that the transition is removed.

  

  



        Attachment #8477646 -
        Flags: feedback-

    
  

        Attachment #8477646 -
        Flags: feedback?(anthony)

    
    

    
  
Wilson, Przemek, Yan, Casey and I met to discuss this morning. I will add comments on the transition itself to bug #1057005.

Not being happy with discussion in another bug is not a reason to set an ultimatum to force a particular UX outcome, which is that the transition must either be disabled or removed, effectively the same outcome and not a choice at all.

Our internal processes do not require feedback+ in order to land. Only review+ and ui-review+ (for items with user facing changes) are required to land. I checked with Jason just now to double check this. 

Flagging Diego to see if he can land this.

  

  


Flags: needinfo?(dmarcos)

    
    

    
  
Comment on attachment 8477646 [details]
Updated PR

(In reply to Stephany Wilkes from comment #36)
> Wilson, Przemek, Yan, Casey and I met to discuss this morning. I will add
> comments on the transition itself to bug #1057005.
> 
> Not being happy with discussion in another bug is not a reason to set an
> ultimatum to force a particular UX outcome, which is that the transition
> must either be disabled or removed, effectively the same outcome and not a
> choice at all.

I thought it was pretty obvious that the transition is out of place and I didn't expect this much disagreement on it or that there was really any discussion at all needed on it. I'm flagging Carrie for ui-review as she handles the dialer. If she's ok with it, then I'll be happy to land it.

> Our internal processes do not require feedback+ in order to land. Only
> review+ and ui-review+ (for items with user facing changes) are required to
> land. I checked with Jason just now to double check this. 

Anthony's review comments were technical in nature so I'm going to request review from him instead. The feedback flag and my comment regarding the transition are two separate issues.

  

  



        Attachment #8477646 -
        Flags: ui-review?(cawang)

        Attachment #8477646 -
        Flags: review?(anthony)

        Attachment #8477646 -
        Flags: review+

        Attachment #8477646 -
        Flags: feedback?(anthony)

        Attachment #8477646 -
        Flags: feedback-

    
    

    
  
I'd also like to point to comment 26 as a source of disagreement. We're not forcing a design on the web component, but the decision within dialer doesn't have to be the same.

  

  



    
    

    
  
This is not Carrie's work. This is Common Controls work, which is handled by Przemek. It is visual design work and not interaction design work, which is what Carrie handles.

  

  



    
    

    
  
Comment on attachment 8477646 [details]
Updated PR

Setting the ui-flag? to Przemek, who does the Common Controls work for the UX team.

  

  



        Attachment #8477646 -
        Flags: ui-review?(cawang) → ui-review?(pabratowski)

    
    

    
  
Also, per comment #26, Carol deferred to Przemek, since this is Common Controls work (as she noted). Przemek spoke with Wilson, cleared his flag, and Wilson carried on. It is not a source of disagreement. Przemek is the UX owner here.

  

  



    
    

    
  
Okay, thanks for clearing that up. I disagree with this but it's not worth it to me to argue further. We still have to wait for Anthony's review, though.

I'm clearing the needinfo on Diego since this still needs review from Anthony.

  

  


Flags: needinfo?(dmarcos)

    
    

    
  
That's fine. And I promise: if this is bad in any way; if we test it on our nightly builds (the entire UX team uses the Flame as our primary devices) and anything looks bad or strange; if it doesn't work or look good on Tarako; we will change it. 

I hope that improved platform support for web components in 2.2 will address a lot of these issues.

  

  



    
    

    
  
Regardless of the transition decision on this button, the accessibility issue I pointed out in comment 30 (bug 887541) is enough to not land this yet. I'm fine landing this if our accessibility team is ok with the regression. But it seems we have a conflict between Ux goals and accessibility goals in 2.1.

  

  



    
    

    
  
(In reply to Anthony Ricaud (:rik) from comment #44)
> Regardless of the transition decision on this button, the accessibility
> issue I pointed out in comment 30 (bug 887541) is enough to not land this
> yet. I'm fine landing this if our accessibility team is ok with the
> regression. But it seems we have a conflict between Ux goals and
> accessibility goals in 2.1.

This has landed. Are we ready to land?

  

  


Flags: needinfo?(anthony)

    
    

    
  
Comment on attachment 8477646 [details]
Updated PR

Sorry, I meant to do that yesterday evening. Yup, with the Gecko patch landed, this is ok to land.

  

  



        Attachment #8477646 -
        Flags: review?(anthony) → review+
Flags: needinfo?(anthony)

    
    

    
  

      
      
      
      

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

    


  
Rebased. Carrying r+ forward.

  

  



        Attachment #8477646 -
        Attachment is obsolete: true

        Attachment #8477646 -
        Flags: ui-review?(pabratowski)

        Attachment #8480481 -
        Flags: review+

    
  
Assignee: yor → wilsonpage

    
    

    
  
Green and ready to land as soon as Gaia opens.

  

  



    
    

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

LANDED https://github.com/mozilla-b2g/gaia/commit/459d6e18271f3eb3a7bb044521e38f092ded2806

  

  



    
  
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1061122

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: