Closed Bug 979487 Opened 6 years ago Closed 6 years ago

[calendar] - Update header design to accommodate larger back-button

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: pivanov, Assigned: pivanov)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Attached file patch for Gaia/master (obsolete) —
Have in mind that this patch depends on bug 979473 and bug 979478
Comment on attachment 8385523 [details] [review]
patch for Gaia/master

Hi Kevin: Please review for the impact on Calendar or reassign as appropriate. Thanks!
Attachment #8385523 - Flags: review?(kgrandon)
Comment on attachment 8385523 [details] [review]
patch for Gaia/master

Pavel - I need you to land the dependent bugs first, or provide a branch that has all of the necessary changes in a single place.
Attachment #8385523 - Flags: review?(kgrandon)
Flags: needinfo?(pivanov)
Hey Kevin,

we need to land all of the bugs together because otherwise we will break all apps.
Here is the branch https://github.com/pivanov/gaia/tree/bb-headers who contains patches from bugs:
  - 979473 [BB]Headers
  - 979478 [BB]Edit_mode
  - 979487 [calendar]

hope this helps

P.S. if you need a branch with all patches for Header changes just ping me :) Thanks in advance :)
Flags: needinfo?(pivanov) → needinfo?(kgrandon)
Sure - but the patch as-is is not easy to review. I think we just haven't really put processes into place for patches like these, but I guess my recommendation would be to have a pull request with multiple commits from the other bugs. (Then all changes are in a single pull request, and as things land you can rebase the branch against master).

Anyway - I'll get to this today, but I do think we need better processes for this type of work.
Flags: needinfo?(kgrandon)
Ok - I took a look, and unfortunately I can't R+ this yet. The headings for the settings tray looks mis-aligned, and also the "Add an Account" header also looks mis-aligned. I'm not a ux designer so I can't ui-review, but I would highly doubt that this is acceptable from a ui-review. I'll try to upload screenshots but my phone is having usb/network problems, ugh. Please let me know if you can't reproduce.
Flags: needinfo?(pivanov)
Hey Kevin I agree with you about the process ... it's very hard to keep up to date all PR but I think that we can do something smarter next time.

as for the misaligned headings I think Casey can help us but for v1.4 this is the spec for the [BB]Headers ... we will do some JS tricks for v1.5
Flags: needinfo?(pivanov) → needinfo?(kyee)
Can you show me the spec? The headers just look wrong for the tray and oauth flows. The others look fine - so I highly doubt that those headers are correct.

I also disagree that we should have any JS for headers - it's just not needed and we can augment the platform if we are missing layout power in CSS.
Patryk - Can you please verify the appearance of the header in the calendar tray? Thanks!
Attachment #8387050 - Flags: ui-review?(padamczyk)
Patryk - Also can you verify the appearance of the header on this page? Thanks!
Attachment #8387051 - Flags: ui-review?(padamczyk)
Kevin, is this a Js solution?   The screen shot in Comment 10 should align the header text to the center of the screen.  It looks to be centered to available space.
Flags: needinfo?(kyee)
Comment on attachment 8387051 [details]
Add Account /w patch.png

Kevin, yes this looks as expected without the JS fix.
Attachment #8387051 - Flags: ui-review?(padamczyk) → ui-review+
Comment on attachment 8387050 [details]
Calendar Tray /w Patch.png

Looks correct.
Attachment #8387050 - Flags: ui-review?(padamczyk) → ui-review+
I am confused. Why are we talking about a JS fix? We don't need a JS fix to do any centering here, and I think this looks bad as it's not centered, and the tray does not follow scan lines.

Patryk/Pavel - Is the opinion that we need a JS fix for centering because this is not the case at all. I can do a PoC to prove this fact.
Flags: needinfo?(pivanov)
Flags: needinfo?(padamczyk)
Comment on attachment 8385523 [details] [review]
patch for Gaia/master

I don't think this looks very good, but I suppose I am ok with landing if visual guys are happy. We should absolutely not use javascript for this layout though - there is no need for this, so ping me if I can help or if you have questions.
Attachment #8385523 - Flags: review+
It should be centered. I was lead to believe we need a JS fix for that. If we don't sweet!
Flags: needinfo?(padamczyk)
Kevin can you make it look like the JS Fix column without the JS changes? And if the label is took long (when it ellipsis) can it then be aligned off center to maximize the space available?
Flags: needinfo?(kgrandon)
(In reply to Patryk Adamczyk [:patryk] UX from comment #17)
> Created attachment 8387913 [details]
> Ideal Solution (look at the JS fix column)
> 
> Kevin can you make it look like the JS Fix column without the JS changes?
> And if the label is took long (when it ellipsis) can it then be aligned off
> center to maximize the space available?

Yes - we can do this. We may need to change dom structure, or be clever about previous/next element selectors if we want a cleaner dom. Which bug is this work being done in? I can take a look at the patch and provide some more details. Is it bug 979473?
Flags: needinfo?(kgrandon) → needinfo?(padamczyk)
Bug 979473 is the meta for the feature. Casey and Pavel did all the work for it so far, so please sync with them on any new code you're trying out.
Flags: needinfo?(padamczyk) → needinfo?(kyee)
we don't need to change the markup anymore
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(pivanov)
Resolution: --- → INVALID
Attachment #8385523 - Attachment is obsolete: true
Flags: needinfo?(kyee)
You need to log in before you can comment on or make changes to this bug.