Closed
Bug 1006118
Opened 11 years ago
Closed 10 years ago
Create gaia-header web component
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(b2g-v2.0 fixed)
RESOLVED
FIXED
2.0 S2 (23may)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | fixed |
People
(Reporter: arnau, Assigned: wilsonpage)
References
Details
Attachments
(1 file, 2 obsolete files)
No description provided.
Reporter | ||
Updated•11 years ago
|
Blocks: gaia-header
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → arnau
Reporter | ||
Comment 1•11 years ago
|
||
Please review this patch together with https://github.com/mozilla-b2g/gaia/pull/18784.patch
Attachment #8417645 -
Flags: review?(21)
Comment 2•11 years ago
|
||
Before I do the review can you do 2 things in the patch ? 1. Add some tests. That's really one of the thing I would like to address with Web Components, having tests for our basic UI elements. 2. Add some comments next to each workarounds with the related bug # on the platform side. The second would make it easier for third party reviewers to understand why the component is done in such a way (like the duplicated inclusion of stylesheets).
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 8417645 [details]
patch in github
Cancelling review in favor of Wilson's patch
Attachment #8417645 -
Flags: review?(21)
Comment 5•11 years ago
|
||
Comment on attachment 8418725 [details]
pull-request (master)
Comments on github. Thanks for doing the work here :)
Attachment #8418725 -
Flags: feedback?(21) → feedback+
Reporter | ||
Updated•11 years ago
|
Assignee: arnau → wilsonpage
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•11 years ago
|
||
- Addressed Github comments. - Rebased on top of bug 1000132 so that styling looks correct.
Is Vivien's feedback+ sufficient to land this or do we need review? from others? Would like to land this soon so we can start porting a couple of apps to it.
Reporter | ||
Updated•11 years ago
|
Attachment #8418725 -
Flags: review?(21)
Comment 9•11 years ago
|
||
Shouldn't we split the new CSS files? As I read it, we're inserting the same stylesheet twice in the document. That may increase memory accordingly.
Reporter | ||
Comment 10•11 years ago
|
||
(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #9) > Shouldn't we split the new CSS files? As I read it, we're inserting the same > stylesheet twice in the document. That may increase memory accordingly. Anthony, there are some things we still need to figure out. We are trying to land this to start trying some options. Once we have :content on the platform we won't need that extra style. We also need to rethink how we copy styles on build time, right know you just link a BB in a document. With web components we'll need another way to do it.
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #9) > Shouldn't we split the new CSS files? As I read it, we're inserting the same > stylesheet twice in the document. That may increase memory accordingly. We all know this is a less than ideal solution, but I agree with Arnau. It will be extra work to split the stylesheets up into 'Shadow' and 'Light', only to (once :content lands) merge them again. Although if we run in to memory issues we may have no choice.
Comment 12•11 years ago
|
||
I meant splitting the stylesheets between the selectors that apply to the current markup and the selectors that will apply to the new web component.
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #12) > I meant splitting the stylesheets between the selectors that apply to the > current markup and the selectors that will apply to the new web component. I agree with that split. Arnau was concerned that if we didn't roll it out over all apps that it would be troublesome to maintain the BB and the WC versions in parallel. Perhaps we should file a follow-up for this?
Comment 14•11 years ago
|
||
(In reply to Anthony Ricaud (:rik) (out until May 13th) from comment #9) > Shouldn't we split the new CSS files? As I read it, we're inserting the same > stylesheet twice in the document. That may increase memory accordingly. It does not really unless someone touch the CSSOM representation. At least that how I understood it by asking to dbaron.
Comment 16•11 years ago
|
||
Comment on attachment 8418725 [details]
pull-request (master)
Will review the patch.
Attachment #8418725 -
Flags: review?(21)
Assignee | ||
Comment 17•11 years ago
|
||
I don't know how to update the .patch thingy. But the PR is up to date.
Comment 18•11 years ago
|
||
Comment on attachment 8420063 [details] [diff] [review] 1006118.patch Review of attachment 8420063 [details] [diff] [review]: ----------------------------------------------------------------- Very close! ::: apps/sharedtest/test/unit/web_components/gaia-header_test.js @@ +26,5 @@ > + test('Should add the correct icon class for the action type', function() { > + this.container.innerHTML = '<gaia-header data-action="menu"></gaia-header>'; > + var element = this.container.firstElementChild; > + var buttonInner = element.shadowRoot.getElementById('action-button-inner'); > + assert.isTrue(buttonInner.classList.contains('icon-menu')); You can probably do a loop for this tests, with the known action type. You may also want a test to check that the button is not set for a unknown action type. @@ +43,5 @@ > + var actionButton = element.shadowRoot.getElementById('action-button'); > + assert.isTrue(HTMLElement.prototype.addEventListener.withArgs('click').calledOn(actionButton)); > + }); > + > + test('Should add the shadow-dom stylesheet to the root of the elmement', function() { s/elmement/element ::: build/preferences.js @@ +46,5 @@ > this.prefs['layout.css.sticky.enabled'] = true; > > + > + > + this.prefs['dom.webcomponents.enabled'] = true; Can you add a comment refering to bug 1000199 in order to know that this pref needs to be removed once it has landed ? ::: shared/web_components/gaia-header/js/component.js @@ +53,5 @@ > + * @private > + */ > + proto.configureActionButton = function() { > + var inner = this.template.getElementById('action-button-inner'); > + var button = this.template.getElementById('action-button'); Let's declare inner closer to where it is used. The allocation is not needed if type is empty. @@ +57,5 @@ > + var button = this.template.getElementById('action-button'); > + var type = this.dataset.action; > + > + if (!type) { > + button.style.display = 'none'; We should really consider making that the default style in the stylesheet and just early return at the top of the method if (!this.dataset.action). Can you open a followup or fix it here ?
Attachment #8420063 -
Flags: review?(21)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8418725 [details]
pull-request (master)
Nits addressed. Don't want to touch CSS are this point as it looks pretty fragile. I'd prefer to make the action-button hidden by default when we make the dedicated web-component stylesheet.
Attachment #8418725 -
Flags: review?(21)
Comment 20•11 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #19) > Comment on attachment 8418725 [details] > pull-request (master) > > Nits addressed. Don't want to touch CSS are this point as it looks pretty > fragile. I'd prefer to make the action-button hidden by default when we make > the dedicated web-component stylesheet. Can you file a followup for that and add a comment in the patch ? r+ with that and the small nit on github.
Assignee | ||
Updated•11 years ago
|
Attachment #8417645 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8420063 -
Attachment is obsolete: true
Attachment #8418725 -
Flags: review?(21) → review+
Assignee | ||
Comment 21•10 years ago
|
||
Landed on 'master' https://github.com/wilsonpage/gaia/commit/ae5620b57bbe0f4968ec6c58640c33f234627470
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
status-b2g-v2.0:
--- → fixed
Target Milestone: --- → 2.0 S2 (23may)
You need to log in
before you can comment on or make changes to this bug.
Description
•