Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

feat (systemaddom): #3148 make the sections (+Top Sites) collapsible #3617

Merged
merged 1 commit into from Oct 6, 2017
Merged

feat (systemaddom): #3148 make the sections (+Top Sites) collapsible #3617

merged 1 commit into from Oct 6, 2017

Conversation

rlr
Copy link
Contributor

@rlr rlr commented Sep 27, 2017 •
edited

Once again, a lot of the code changes is moving stuff around.

Fixes #3148

@rlr rlr force-pushed the gh3148/collapsible-sections branch 6 times, most recently from a203e78 to a84dab5 Compare October 3, 2017 20:33
@rlr rlr requested review from k88hudson and csadilek October 4, 2017 11:30
@rlr rlr force-pushed the gh3148/collapsible-sections branch from a84dab5 to b0c07cd Compare October 4, 2017 18:53
csadilek
csadilek suggested changes Oct 5, 2017
View reviewed changes
Copy link
Collaborator

@csadilek csadilek left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well! Only found one issue: A collapsed section should not send view impressions but currently does, see dispatchImpressionStats().

So, we'd need to find a way to send view impressions when the section is expanded, similar to what we do with the visibility change handler.

Mardak
Mardak reviewed Oct 5, 2017
View reviewed changes
system-addon/content-src/styles/_icons.scss
background-image: url('#{$image-path}glyph-arrowhead-down-12.svg');
background-size: $smaller-icon-size;
height: $smaller-icon-size;
transform: rotate(270deg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need 90deg for rtl?

@rlr rlr force-pushed the gh3148/collapsible-sections branch from b0c07cd to 9d28eae Compare October 5, 2017 20:32
@rlr
Copy link
Contributor Author

rlr commented Oct 5, 2017

@csadilek I've addressed the impressions issue here 9d28eae

@csadilek
Copy link
Collaborator

csadilek commented Oct 5, 2017

@rlr looking good. Found one issue when testing the update: After collapsing the section, I get one more view impression ping when opening a new tab, subsequent new tabs are fine. Reopening/Expanding the section works fine too! Turned ping-centre.log on to see this. Happy to help debug this.

@csadilek
Copy link
Collaborator

csadilek commented Oct 6, 2017 •
edited

@rlr I've just verified that this one additional ping happens after any pref change. So, it's unrelated to your changes. I will take a look and file a follow-up issue, if needed.

UPDATE: Argh, I see it now. We need an additional check for && !isCollapsed here: https://github.com/rlr/activity-streams/blob/9d28eae56b980d89ddfcd2383322d341c695315e/system-addon/content-src/components/Sections/Sections.jsx#L55

The visibility listener could've been attached in a tab where the section was still expanded. So, the section could've been collapsed in the meantime. This only happens for pre-rendered tabs. That's why I was only seeing one additional ping. Sorry for my confusion earlier.

csadilek
csadilek suggested changes Oct 6, 2017
View reviewed changes
Copy link
Collaborator

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one minor issue (see updated comment above).

@rlr rlr force-pushed the gh3148/collapsible-sections branch from 9d28eae to 30c4613 Compare October 6, 2017 15:31
@rlr rlr force-pushed the gh3148/collapsible-sections branch from d5e790a to edd41d3 Compare October 6, 2017 15:54
@rlr rlr merged commit e708b26 into mozilla:master Oct 6, 2017
@as-pine-proxy
Copy link
Collaborator

Treeherder: 15fbf909aa4907b551120b206f17e66fdc87aae7

@Mardak Mardak mentioned this pull request Oct 10, 2017
Closed
@rlr rlr deleted the gh3148/collapsible-sections branch November 18, 2017 01:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mardak Mardak Mardak left review comments

@csadilek csadilek csadilek approved these changes

@k88hudson k88hudson Awaiting requested review from k88hudson

Assignees

@k88hudson k88hudson

Labels
PR / Needs review pushed-to-pine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rlr @csadilek @as-pine-proxy @Mardak @k88hudson