-
Notifications
You must be signed in to change notification settings - Fork 4
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
Make the icons in application menu configurable (external services, metadata domains) #1519
Conversation
if (object.iconPath === null) { | ||
return Promise.resolve(object); | ||
} | ||
return axios.get(object.iconPath, {responseType: 'blob'}).then(response => { |
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.
do I understand correctly, we don't cache now the icons? They are pretty static, something to make sense to cache in browser. What do you think?
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.
Yes, you're right, I had a plan to add caching, but then forgot about it! Thanks, I'll add it!
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.
It is added now
projects/mercury/src/metadata/metadata-sources/InternalMetadataSourceContext.js
Show resolved
Hide resolved
@@ -44,11 +46,25 @@ public static class Storage { | |||
private String rootDirectoryIri; | |||
} | |||
|
|||
@Data | |||
@JsonInclude(JsonInclude.Include.NON_NULL) | |||
public static class Service { |
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.
I would name it more specifically, something like ExternalService, otherwise it is a bit general
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.
I can try different name... This is how we call this option in the configuration, just services
, and I just moved it from Saturn to Pluto.
projects/pluto/src/main/java/nl/fairspace/pluto/web/IconsResource.java
Outdated
Show resolved
Hide resolved
|
||
@RestController | ||
@Slf4j | ||
@AllArgsConstructor |
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.
Resource is a singleton and unmodifiable. This is correct to define fields final, @RequiredArgsConstructor
can be used instead
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.
Does it matter in this case if I use RequiredArgsConstructor vs AllArgsConstructor?
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.
not critical, the only advantage is that it will be safer to add new dependencies
|
||
private final PlutoConfig plutoConfig; | ||
/** | ||
* GET /api/iconsvg/{icon_name} : returns an icon of specified name if exists. |
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.
@return
tag can be used for proper layout and common practice
} | ||
} | ||
|
||
public String getIconUrl(String name) { |
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.
If to leave it here, I would suggest to make it private as it is for internal usage. But rather I would suggest to make PlutoConfig responsible for the methods and exposing them. Resource/Controller layer should be responsible only for accepting a request and routing it further to dedicated service
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 one is not only used internally and PlutoConfig is more of a DTO, so I don't think methods belong there. I will see how to organize this better
@@ -33,7 +34,8 @@ public ResponseEntity<List<MetadataSourceInfo>> metadataSources() { | |||
.map(source -> new MetadataSourceInfo( | |||
source.getName(), | |||
source.getLabel(), | |||
String.format("/api/metadata-sources/%s", source.getName()))) | |||
source.getName() != null ? String.format("/api/metadata-sources/%s", source.getName()) : null, |
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.
I would put the parametrized path to const
ef914aa
to
f0bab73
Compare
Overall looks good! The only thing I don't get is why services configuration is moved from Saturn to Pluto, can you explain a bit because it makes it more complex than it used to be. |
My goal was to simplify things actually :) |
Implements FAIRSPC-50
Unfortunately there is no way to include external files in helm install/upgrade command, so the current workaround is to pass icon files one by one with
--set-file
option (as I described in the readme file). Hopefully this feature will be included in helm, since there is already a PR for that:helm/helm#10077 (unfortunately waiting for 4 years now...).