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

RESOLVED FIXED in Firefox 67

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: jsantell, Assigned: lloanalas, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [good first bug][lang=css][lang=js])

Attachments

(3 attachments)

Not sure how exactly, but atleast have a `pointer` cursor to make it look clicky.
Has STR: --- → irrelevant

Comment 1

3 years ago
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 .

Updated

3 years ago
Flags: needinfo?(jsantell)

Updated

3 years ago
Assignee: nobody → deepesh910
Status: NEW → ASSIGNED

Comment 2

3 years ago
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.
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
Flags: needinfo?(jsantell)

Comment 4

3 years ago
Hello Jordan,

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

Thank You.
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug

Comment 6

3 years ago
If this is still open I would love to work on this bug. Would it be possible to be assigned it?
I really want to give this a shot. Can I have this assigned?
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 :)
Flags: needinfo?(jordan)
Thank you for the patch! I'm not really sure of the status of this bug, pinging Greg who will have a better idea.
Flags: needinfo?(jordan) → needinfo?(gtatum)
Attachment #8868356 - Flags: review?(jordan)

Comment 11

2 years ago
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

2 years ago
(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!
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
Flags: needinfo?(gtatum)

Updated

a year ago
Product: Firefox → DevTools

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.

Assignee: deepesh910 → nobody
Status: ASSIGNED → NEW
Assignee

Comment 15

3 months ago

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!

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?

Assignee: nobody → lloanalas
Assignee

Comment 17

3 months ago

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

Assignee

Comment 18

3 months ago

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

(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!

Mentor: jordan → felash
Assignee

Comment 20

3 months ago

:Julienw

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

Thank you!

Flags: needinfo?(felash)

I approved and pushed it, thanks a lot!

Flags: needinfo?(felash)
Assignee

Comment 22

3 months ago

(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

3 months ago
Pushed by jwajsberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22825edb005f
Load More row in dominator trees should be styled differently. r=julienw

Comment 24

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.