Skip to content

Comments

section name for imported libraries#277

Open
safirex wants to merge 5 commits intogluonhq:masterfrom
safirex:master
Open

section name for imported libraries#277
safirex wants to merge 5 commits intogluonhq:masterfrom
safirex:master

Conversation

@safirex
Copy link

@safirex safirex commented Jun 3, 2020

A minor update changing the section names of imported librairies for more visibility

from
image
to
image

@abhinayagarwal
Copy link
Collaborator

Hi @safirex ,

Are you still willing to work on this PR?

@safirex safirex marked this pull request as draft December 27, 2020 00:32
@safirex
Copy link
Author

safirex commented Dec 31, 2020

@abhinayagarwal
hello,
not at the moment, no.

@safirex safirex marked this pull request as ready for review March 19, 2021 19:22
@safirex safirex force-pushed the master branch 3 times, most recently from 044ab07 to c66d969 Compare March 23, 2021 23:17
@safirex safirex requested a review from johanvos March 24, 2021 00:06
@johanvos johanvos requested a review from abhinayagarwal March 24, 2021 07:00
@abhinayagarwal abhinayagarwal force-pushed the master branch 2 times, most recently from 7c0a0f0 to fd0a426 Compare March 30, 2021 03:22
@abhinayagarwal
Copy link
Collaborator

Hi @safirex ,

I have left some comments.

I couldn't find you in our contributors list. Can you please sign the Gluon Individual Contributor License Agreement.

@safirex safirex requested a review from abhinayagarwal April 2, 2021 08:03
@abhinayagarwal
Copy link
Collaborator

Looks good. One more change which I missed to point out earlier. Please update the license header on the file to current year:

Copyright (c) 2017, 20202021, Gluon and/or its affiliates

if (!excludedItems.contains(canonicalName) &&
!artifactsFilter.contains(canonicalName)) {
final String name = e.getKlass().getSimpleName();
final String sectionName = createSectionName(jarOrFolderReport);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be outside the for loop

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, José!

@jperedadnr
Copy link
Collaborator

In general this PR looks good, providing a section name to categorise all different custom controls definitely would help.

However, if the user happens to have many imported jars, the list grows at the top level, and the number of root categories in the accordion is too big, causing the titled panes not to show any content or barely one or two items of a possibly long list. Also, this will clutter the view, having the built-in categories (like Controls) at the end and barely accessible.

To move this PR forward, we need somehow to introduce the concept of sub-category.

As the library items are currently displayed in a titled pane, using a single ListView<LibraryListItem>, I suggest a few options:

  • Use the subcategory title as a Label node, having a ListView control for each jar.
  • Create a LibraryListItem to be used as header, for each subcategory, so a single listView will do, however this will require two different custom cells, and figuring out how to sort it (first by sub-category, then by name).
  • Based on the above, the CharmListView control is already designed for that. The required refactoring is fairly easy to do.

If we keep the accordion as is (with each custom library as a top categories and no sub-categories), we could add a button to the right of the Search field, so the user can manage the categories, hiding the ones not frequently used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants