-
Notifications
You must be signed in to change notification settings - Fork 221
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
base: master
Are you sure you want to change the base?
Conversation
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
Hi @safirex , Are you still willing to work on this PR? |
@abhinayagarwal |
044ab07
to
c66d969
Compare
7c0a0f0
to
fd0a426
Compare
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
kit/src/main/java/com/oracle/javafx/scenebuilder/kit/library/user/LibraryFolderWatcher.java
Outdated
Show resolved
Hide resolved
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. |
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, |
@@ -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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, José!
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
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. |
A minor update changing the section names of imported librairies for more visibility
from
to