Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

section name for imported libraries #277

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

@@ -438,15 +438,22 @@ else if (Files.isDirectory(currentJarOrFolder)) {
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