Open Bug 1403203 Opened 2 years ago Updated 24 days ago

Use Accordion component for the side-bar

Categories

(DevTools :: Netmonitor, task, P3)

task

Tracking

(firefox57 fix-optional)

Tracking Status
firefox57 --- fix-optional

People

(Reporter: Honza, Unassigned, Mentored)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files)

Net monitor side bar is built on top of a tree view component. We should refactor it and base on top of Accordion component [1]. This component is supposed to be shared across tools and make the UI/UX consistent [2].


The component should be moved into shared directory [3], but this doesn't have to happen in this bug.


[1] http://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/components/Accordion.js

[2] http://searchfox.org/mozilla-central/source/devtools/client/inspector/layout/components/Accordion.css

[3] devtools/client/shared/components
Product: Firefox → DevTools
I would like to work on this. Can I ?
Flags: needinfo?(odvarko)

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #1)

I would like to work on this. Can I ?

Yes!

Netork monitor sidebar has several side panels almost all of them (except of the Timings panel, I think) are using PropertiesView component (based on TreeView) to render sections and content.

You could perhaps start with simpler panels, like e.g. the CookiesPanel:

https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/CookiesPanel.js

Another approach could be to reimplement the PropertiesView and build it on top of the Accordion instead of TreeView.

Can you please start with some analysis?

Honza

Assignee: nobody → hems.india1997
Mentor: odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko) → needinfo?(hems.india1997)

Thanks for assigning this to me. I had a brief overview of the components and the PropertiesView component. I feel that rather making the changes one by one, it would be better to reimplement PropertiesView on top of the Accordion component. I will start looking at the code in depth and come back with my thoughts about how to proceed with this.

Thanks,
Hemanth

I had a look at the code of both Accordion and PropertiesView and TreeView components. After doing some digging, what I feel is that the functionality provided by TreeView is not provided by Accordion. So, what was the intention behind this bug? Is it changing the behaviour of these panels or keeping the behaviour intact and just refactoring and building it upon a different component. If we want to keep behavior intact, then how should we be doing it? Should we build something on top of Accordion which will give us the same functionality as TreeView and use that to build PropertiesView? Or is there a better and easier approach for this?

Flags: needinfo?(hems.india1997) → needinfo?(odvarko)

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #4)

I had a look at the code of both Accordion and PropertiesView and TreeView components. After doing some digging, what I feel is that the functionality provided by TreeView is not provided by Accordion.

Correct, we need to combine/use these two components.

So, what was the intention behind this bug? Is it changing the behaviour of these panels or keeping the behaviour intact and just refactoring and building it upon a different component.

Keep the behavior intact

If we want to keep behavior intact, then how should we be doing it?

So, we need both components.

Most of the Network's side panels are having more sections. For example, the Headers panel has: Response Headers, Request Headers and Headers from Upload Stream. The Cookies panel has Request & Response Cookies etc. All these sections are built on top of the Properties view, which is built on top of the Tree View. This is not correct. The sections itself should be built on top of the Accordion component and their content (e.g. the actual list of HTTP headers or list of Cookies) should be built using the Properties view.

All the logic (in PropertiesView) that relates to the sections should be removed (since provided by the Accordion) and the Properties view should be responsible for building the lists (i.e. content) only.

Does that make sense?

Honza

Flags: needinfo?(odvarko)

Yes, That does make a lot of sense :). I will start thinking about this and come back with a plan :)

All the logic (in PropertiesView) that relates to the sections should be removed

Does this mean that we have to modify the logic in the PropertiesView component itself or are you talking about the logic that is generating the PropertiesView components for a particular panel?

Flags: needinfo?(odvarko)

I looked at the code for these panels. There was only one PropertyView object, which in turn has one TreeView object and this TreeView object had rows ( which are each of these sections in a panel ). So now, I am confused on what you mean't by the following the statement.

All these sections are built on top of the Properties view, which is built on top of the Tree View. This is not correct. The sections itself should be built on top of the Accordion component and their content (e.g. the actual list of HTTP headers or list of Cookies) should be built using the Properties view.

Did I misunderstand something?

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #8)

I looked at the code for these panels. There was only one PropertyView object, which in turn has one TreeView object and this TreeView object had rows ( which are each of these sections in a panel ).

Yes, this is correct. The TreeView is used to render even the (expandable/collapsible) section-labels -> mimicking behavior of an Accordion.

The goal here is:

  1. Use the Accordion component to render individual sections in the side panels. This is what Accordion is for and we should use it.

  2. Use the TreeView/Properties view to render only content of the (Accordion) sections - e.g. list of HTTP Headers, cookies, etc.

Does this help?

Honza

Flags: needinfo?(odvarko)

So, as I understand the hierarchy looks something like this right?

  • Accordion0 ( For whole panel )
    • Accordion1 ( For section1 )
      • TreeView1 (for content of section1)
    • Accordion2 ( For section2 )
      • TreeView2 ( for content of section2 )
    • ...

Is this how we intend it to be?

Flags: needinfo?(odvarko)

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #10)

Is this how we intend it to be?

Correct

Honza

Flags: needinfo?(odvarko)

For headers and cookie panel, the PropertiesView class renders a searchbox. Should we add this logic to the individual panels itself when we stop using the PropertiesView class to render the whole panel?

Flags: needinfo?(odvarko)

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #12)

For headers and cookie panel, the PropertiesView class renders a searchbox. Should we add this logic to the individual panels itself when we stop using the PropertiesView class to render the whole panel?

(sorry for the delay)

I think so yes. Of course, we should share the logic across panel
as much as possible (to avoid code duplication).

Honza

Flags: needinfo?(odvarko)

Thanks for the reply Honza. This is my plan currently.

  • The section panels will continue to create PropertiesView object.
  • PropertiesView object will instead create Accordion objects in the following manner
  • Accordion for whole panel.
    • Accordion0 for search box ( if necessary )
    • Accordion1 ( For section1 )
      • TreeView1 (for content of section1)
    • Accordion2 ( For section2 )
      • TreeView2 ( for content of section2 )
      • ...
        I thought this would be better because, in this way all the share logic will continue to stay with PropertiesView and we will still . Does this sound good? Sorry if I am iterating back and forth with the same idea.
Flags: needinfo?(odvarko)

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #14)

I thought this would be better because, in this way all the share logic will continue to stay with PropertiesView and we will still . Does this sound good?

Yes, this sounds correct.

Note that some other panels are also using Accordeon, perhaps you could get some inspiration..

Here is a search link:
https://searchfox.org/mozilla-central/search?q=accordion&path=

Focus on directories under devtools/client/

Honza

Flags: needinfo?(odvarko)

Refactor the PropertiesView component (which is used by all the net monitor components) and build it on top of
Accordion component to make the UI consistent. Note that, the commit is still incomplete.

I made changes so that everything works like before. We might have to style a bit to make it look like before. PTAL and let me know what should be changed.

Flags: needinfo?(odvarko)

I have adjusted the styling as well.

Commenting in Phab.
Honza

Flags: needinfo?(odvarko)
Attached image image.png

Attaching a screenshot of the Scopes Debugger side section. This section is placing a checkbox in the section title, which is something we want to do as well (but insert a toggle button - raw/formatted headers)

STR about how to get that panel

  1. Load http://janodvarko.cz/tests/bugzilla/1539142/
  2. Select the debugger panel
  3. Click Start Worker
  4. Open Webpack/src/worker.js file
  5. Create BP on line 17, wait for break
  6. Check out the side panel, there should be Scopes section with a checkbox.

Honza

But the code you pointed out doesn't seem like an Accordion item. I tried to include it as a button and do event.StopPropagation. It didn't solve the problem. Am I missing something here?

Flags: needinfo?(odvarko)

After some trying, I have been able to make it work. I have updated the patch. PTAL
Thanks,
harry7

(In reply to Hemanth Kumar Veeranki [:harry7] from comment #22)

But the code you pointed out doesn't seem like an Accordion item. I tried to include it as a button and do event.StopPropagation. It didn't solve the problem. Am I missing something here?

Can you check out whether the Debugger project implementing its own Accordion component?

Honza

Flags: needinfo?(odvarko) → needinfo?(harry7.opensource)

Honza, I think the diff in https://phabricator.services.mozilla.com/D22813 requires a review from you. Looks like it was updated recently.

Flags: needinfo?(odvarko)

Review done, sorry for the delay.

Honza

Flags: needinfo?(odvarko)
Type: enhancement → task

Hi harry7, do you still intend to work on this bug? Honza did a review of your code changes some time ago so it'd be great to push it over the finish line.
If you don't have time anymore or don't want to, that's fine! Just let us know so we can make this bug available for others to pick up.

Assignee: harry7.opensource → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(harry7.opensource)
You need to log in before you can comment on or make changes to this bug.