Closed
Bug 1011601
Opened 10 years ago
Closed 10 years ago
[Dialer] Update to use <gaia-header>
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(ux-b2g:2.1)
RESOLVED
FIXED
ux-b2g | 2.1 |
People
(Reporter: yor, Assigned: wilsonpage)
References
Details
Attachments
(1 file, 4 obsolete files)
No description provided.
Updated•10 years ago
|
Blocks: gaia-header
Attachment #8425211 -
Flags: feedback?(wilsonpage)
Attachment #8425211 -
Flags: feedback?(kgrandon)
Attachment #8425211 -
Flags: feedback?(arnau)
Attachment #8425211 -
Flags: feedback?(21)
Comment 2•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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)
Attachment #8429310 -
Flags: review?(wilsonpage)
Attachment #8429310 -
Flags: review?(anthony)
Attachment #8429310 -
Flags: review?(21)
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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?
Comment 14•10 years ago
|
||
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!
Reporter | ||
Comment 16•10 years ago
|
||
Comment on attachment 8429310 [details]
Pull Request
Anthony,
Rebased off latest master. Please review. Thanks.
Attachment #8429310 -
Flags: review+ → review?(anthony)
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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-
Reporter | ||
Comment 19•10 years ago
|
||
Confirmed that call_log.js and suggestions_list.js use of headers are fine.
Reporter | ||
Comment 20•10 years ago
|
||
Comment on attachment 8429310 [details]
Pull Request
Updated patch based on feedback.
Attachment #8429310 -
Flags: review- → review?(drs+bugzilla)
Comment 21•10 years ago
|
||
Comment on attachment 8429310 [details]
Pull Request
See comments on PR.
Attachment #8429310 -
Flags: review?(drs+bugzilla) → review-
Assignee | ||
Comment 22•10 years ago
|
||
UPDATED - Fixed incorrect disabled button style
Attachment #8429310 -
Attachment is obsolete: true
Attachment #8476645 -
Flags: review?(drs+bugzilla)
Reporter | ||
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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)
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
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)
Assignee | ||
Comment 27•10 years ago
|
||
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)
Comment 30•10 years ago
|
||
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-
Comment 31•10 years ago
|
||
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?
Comment 32•10 years ago
|
||
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.
Attachment #8477646 -
Flags: review+
Attachment #8476645 -
Attachment is obsolete: true
Reporter | ||
Comment 34•10 years ago
|
||
Updated PR based on Anthony's feedback and carrying the r+ over. Ready to land.
Comment 35•10 years ago
|
||
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)
Comment 36•10 years ago
|
||
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 37•10 years ago
|
||
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-
Comment 38•10 years ago
|
||
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.
Comment 39•10 years ago
|
||
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 40•10 years ago
|
||
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)
Comment 41•10 years ago
|
||
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.
Comment 42•10 years ago
|
||
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)
Comment 43•10 years ago
|
||
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.
Comment 44•10 years ago
|
||
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.
Assignee | ||
Comment 45•10 years ago
|
||
(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 46•10 years ago
|
||
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)
Assignee | ||
Comment 47•10 years ago
|
||
Rebased. Carrying r+ forward.
Attachment #8477646 -
Attachment is obsolete: true
Attachment #8477646 -
Flags: ui-review?(pabratowski)
Attachment #8480481 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Assignee: yor → wilsonpage
Assignee | ||
Comment 49•10 years ago
|
||
Comment on attachment 8480481 [details] pull-request (master) LANDED https://github.com/mozilla-b2g/gaia/commit/459d6e18271f3eb3a7bb044521e38f092ded2806
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•