1242003 - "Load More..." row in dominator trees should be styled differently

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P3)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: Screen Shot 2016-01-21 at 10.33.00 PM.png

Not sure how exactly, but atleast have a `pointer` cursor to make it look clicky.

Comment 1

[deepesh910]

I would like to fix this bug. However this would be my first bug and I have no idea if i have adequate knowledge to debug it . I would try my best howsoever. Please assign this bug to me .

Comment 2

[deepesh910]

Since this is my first bug , i need some help . Where do i exactly find the code which i have to de-bug ?please help.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

The "load more" frame can be found in devtools/client/memory/components/dominator-tree.js -- the `.load-more` class can be styled in devtools/client/themes/memory.css

Comment 4

[Wasim Thabraze]

Hello Jordan,

I would like to work on this bug. I'll try to submit a patch as soon as I can.

Thank You.

Comment 5

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 6

[liamsgarvie]

If this is still open I would love to work on this bug. Would it be possible to be assigned it?

Comment 7

[[e-mail]]

I really want to give this a shot. Can I have this assigned?

Comment 8

[Santiago Paez [:tiago]]

Hi!

I uploaded a patch for this. 

I'm not sure if the .load-more class I implemented has the expected behavior,
if not, just let me know which properties are expected and I'll add them right away :)

Comment 9

[Santiago Paez [:tiago]]

Attachment: Bug 1242003 - Update style of 'Load more...' row in dominator trees on devtools:memory.

Review commit: https://reviewboard.mozilla.org/r/139926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139926/

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Thank you for the patch! I'm not really sure of the status of this bug, pinging Greg who will have a better idea.

Comment 11

[Rayann Tedds]

I would like to work on this bug as part of my university coursework. Please could I be assigned to it and given extra information? Thank you!

Comment 12

[Rayann Tedds]

(In reply to Rayann Tedds from comment #11)
> I would like to work on this bug as part of my university coursework. Please
> could I be assigned to it and given extra information? Thank you!

Also, how do I actually get to review the bug - is there a HTML file to view or something?

Thank you!

Comment 13

[Greg Tatum [:gregtatum]]

Sorry, this fell of my radar. The existing attached patch may actually work for it (although by now it's a kind of stale). This tool is a bit on "maintenance mode" with only fixing regressions. If you're looking for some easy first patches we're actively working on perf.html for performance work in Firefox:

https://github.com/devtools-html/perf.html/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22

Comment 14

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 6 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 15

[lloan alas:[lloanalas]]

Hi, I would like to have this bug assigned to me.

According to comment #10, the JS code for the "load more" frame can be found in devtools/client/memory/components/dominator-tree.js and the .load-more class can be modified/added in 'devtools/client/themes/memory.css'.

Thanks!

Comment 16

[Julien Wajsberg [:julienw]]

Hey lloanalas, yes please :)

As comment 0 says, it should get a "pointer" cursor on hover. And probably we should make it look a bit more like a link (as we don't have enough space for a button), so please follow the style outlined in https://design.firefox.com/photon/components/links.html. Maybe we should even make it a link at all?

Comment 17

[lloan alas:[lloanalas]]

Julien,

I was reading the style outline and it seems like links serve a different purpose. The 'load more' text does trigger a change in the view - wouldn't that call for the use of a button? Is the issue using a button that the size of the button would be too small? https://i.imgur.com/5tjaiMZ.png

Comment 18

[lloan alas:[lloanalas]]

Attachment: Bug 1242003 - Load More row in dominator trees should be styled differently. r=julienw

Updated "Load More" row in dominator trees to look more like a link in order to tell it apart from other text and to indicate that the element can be clicked. Not enough space

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to lloanalas from comment #17)

Julien,

I was reading the style outline and it seems like links serve a different purpose. The 'load more' text does trigger a change in the view - wouldn't that call for the use of a button? Is the issue using a button that the size of the button would be too small? https://i.imgur.com/5tjaiMZ.png

Yes, that's a good remark. This was also part of my thinking initially. According to the doc, the minimum height of a button is 24px, and we don't have this space, so we'd need to make it smaller, and I don't think this would be nice in the end. So that's why I recommended to use a link: even if it's not a perfect solution, I think it's the best we can do here.

I'll look at your patch shortly, thanks!

Comment 20

[lloan alas:[lloanalas]]

:Julienw

I updated the items you pointed out. Could you take a look when you get a chance?

Thank you!

Comment 21

[Julien Wajsberg [:julienw]]

I approved and pushed it, thanks a lot!

Comment 22

[lloan alas:[lloanalas]]

(In reply to Julien Wajsberg [:julienw] from comment #21)

I approved and pushed it, thanks a lot!

Great! Thanks - learned quite a bit about the branding and where to find those items within the code base. Thanks - very useful!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22825edb005f
Load More row in dominator trees should be styled differently. r=julienw

Comment 24

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/22825edb005f