Closed Bug 1383735 Opened 2 years ago Closed 2 years ago

Activity Stream: Update highlights layout to match latest mocks

Categories

(Firefox for Android :: General, enhancement, P1)

All
Android
enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.27
Tracking Status
firefox57 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

()

Details

(Whiteboard: [MobileAS] 1.27)

Attachments

(1 file)

No description provided.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Sebastian, it looks like bug 1379021 (which seems to have addressed the highlights layout) was closed but bug 1380639 (ConstraintLayout to the build system) was not - what changes still need to be addressed for this bug?

We're trying to figure out what's been unblocked from the P2 list with respect highlights.
Flags: needinfo?(s.kaspari)
Iteration: --- → 1.27
Priority: P2 → P1
Whiteboard: [MobileAS] → [MobileAS] 1.27
I created those bugs (highlights / top sites) as follow-up bugs because I couldn't land all changes without ConstraintLayout.
Flags: needinfo?(s.kaspari)
Depends on: 1380639
Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

https://reviewboard.mozilla.org/r/164552/#review170106
Attachment #8893470 - Flags: review?(michael.l.comella) → review+
I can't land this anymore. ConstraintLayout is making issues - see bug 1388023. I had the sheriffs remove it from central again: bug 1380639.

I'll rewrite this to use regular layouts and then we can migrate later.
Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

@mcomella: Can you have a look at the updated patch (I'm not sure how to re-request it from mozreview). This is a version that does not use ConstraintLayout anymore. Luckily I was able to simplify the layout nevertheless.
Attachment #8893470 - Flags: review+ → review?(michael.l.comella)
Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

https://reviewboard.mozilla.org/r/164552/#review170922

::: mobile/android/app/src/main/res/layout/activity_stream_card_history_item.xml:20
(Diff revision 2)
> -        <FrameLayout
> +    <FrameLayout
> -            android:id="@+id/icon_wrapper"
> +        android:id="@+id/icon_wrapper"
> -            android:layout_width="wrap_content"
> +        android:layout_width="wrap_content"
> -            android:layout_height="wrap_content">
> +        android:layout_height="wrap_content">
>  
> -            <org.mozilla.gecko.widget.FaviconView
> +        <org.mozilla.gecko.widget.FaviconView

This probably doesn't need a contentDescription, right? This discussion could also go into one of the a11y bugs.

::: mobile/android/app/src/main/res/layout/activity_stream_card_history_item.xml:41
(Diff revision 2)
> -            android:layout_margin="2dp"
> -            android:layout_alignParentEnd="true"
> +        android:layout_alignParentEnd="true"
> -            android:layout_alignParentRight="true"
> +        android:layout_alignParentRight="true"
> -            android:layout_alignParentTop="true"
> +        android:layout_alignParentTop="true"
> -            android:layout_gravity="right|end|top"
> +        android:layout_gravity="right|end|top"
> +        android:layout_margin="0dp"

nit: Is margin = 0 necessary? I thought it was the default value.

::: mobile/android/app/src/main/res/layout/activity_stream_card_history_item.xml:45
(Diff revision 2)
> -            android:layout_gravity="right|end|top"
> +        android:layout_gravity="right|end|top"
> +        android:layout_margin="0dp"
> -            android:contentDescription="@string/menu"
> +        android:contentDescription="@string/menu"
> -            android:src="@drawable/menu"
> -            android:padding="@dimen/activity_stream_base_margin" />
> +        android:paddingBottom="16dp"
> +        android:paddingLeft="0dp"
> +        android:paddingRight="0dp"

nit: same – is specifying 0 necessary? Or perhaps this is for clarity?
Attachment #8893470 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8893470 [details]
Bug 1383735 - Activity Stream: Update highlights layout to match latest mocks.

https://reviewboard.mozilla.org/r/164552/#review170922

> This probably doesn't need a contentDescription, right? This discussion could also go into one of the a11y bugs.

Yeah, I think the favicon is only a indicator that helps you (visually) navigate.

> nit: Is margin = 0 necessary? I thought it was the default value.

You are right. This is an artifact from iterating on the design.
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/450405f6daaa
Activity Stream: Update highlights layout to match latest mocks. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/450405f6daaa
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.