-
Notifications
You must be signed in to change notification settings - Fork 121
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
[BBC] Make additional fields visible on the UI via configuration #3183
Conversation
1c57811
to
f08b18a
Compare
We'll do a code review shortly but wanted to put this here as a general comment on the structure of the configuration. We think that the configuration can be more generic. Specifically:
Taking your example:
# FileMetadata configurations that are used to define which metadata field is visible, searchable and has an alias on the UI
fieldAliases [
{
# Sets the image metadata tag to be used when filtering FileMetadata field
elasticsearchPath = "fileMetadata.xmp.bbc:Programme-Maker"
# Sets an alias for use in search, shouldn't have whitespace or special characters
alias = "BBCProgrammeMaker"
# Sets a human readable label that will rendered on the UI
label = "BBC Programme Maker"
# Sets the field alias to be displayed as a search hint when you type `+`
displaySearchHint = false
},
{
elasticsearchPath = "fileMetadata.xmp.aux:Lens"
alias = "auxLens"
label = "Aux Lens"
displaySearchHint = false
},
{
elasticsearchPath = "fileMetadata.xmp.CreatorTool"
alias = "creatorTool"
label = "Creator Tool"
displaySearchHint = false
},
{
elasticsearchPath = "fileMetadata.exif.Artist"
alias = "artist"
label = "Artist"
displaySearchHint = true
}
] |
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.
A few thoughts for you to look at. We're very happy to run through in a call if you have any questions or want to discuss further.
common-lib/src/main/scala/com/gu/mediaservice/lib/config/CommonConfig.scala
Outdated
Show resolved
Hide resolved
kahuna/public/js/components/gr-image-metadata/gr-image-metadata.js
Outdated
Show resolved
Hide resolved
kahuna/public/js/components/gr-image-metadata/gr-image-metadata.js
Outdated
Show resolved
Hide resolved
kahuna/public/js/components/gr-image-metadata/gr-image-metadata.js
Outdated
Show resolved
Hide resolved
common-lib/src/main/scala/com/gu/mediaservice/lib/config/FileMetadataConfig.scala
Outdated
Show resolved
Hide resolved
common-lib/src/main/scala/com/gu/mediaservice/lib/config/FileMetadataConfig.scala
Outdated
Show resolved
Hide resolved
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.
Morning @ochiengolanga. This PR is in good shape and very close to being ready. Comments are mainly tidying up this time around. With a little refactor of the ImageResponse
code. Let me know if you have any questions.
If you're able to update the description of the PR to match the updated code that would be super helpful.
common-lib/src/main/scala/com/gu/mediaservice/lib/config/FieldAliasConfig.scala
Outdated
Show resolved
Hide resolved
common-lib/src/main/scala/com/gu/mediaservice/model/Image.scala
Outdated
Show resolved
Hide resolved
common-lib/src/main/scala/com/gu/mediaservice/model/FileMetadata.scala
Outdated
Show resolved
Hide resolved
common-lib/src/main/scala/com/gu/mediaservice/lib/config/CommonConfig.scala
Outdated
Show resolved
Hide resolved
kahuna/public/js/components/gr-image-metadata/gr-image-metadata.html
Outdated
Show resolved
Hide resolved
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 is a really nice piece of additional functionality. Thanks @ochiengolanga!
This is fundamentally ready to go. I left a couple of minor comments but I'd be OK merging it as it is.
Please do go ahead and squash your commits and then I'll look to merge shortly after that.
kahuna/public/js/components/gr-image-metadata/gr-image-metadata.js
Outdated
Show resolved
Hide resolved
kahuna/public/js/components/gr-image-metadata/gr-image-metadata.js
Outdated
Show resolved
Hide resolved
common-lib/src/main/scala/com/gu/mediaservice/model/Image.scala
Outdated
Show resolved
Hide resolved
98d36ce
to
f51f6d3
Compare
Hi @ochiengolanga, |
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.
9e2c88f
to
5cf1ce2
Compare
Seen on usage, collections, auth, image-loader-projection (created by @ochiengolanga and merged by @sihil 8 minutes and 36 seconds ago) Please check your changes! |
Seen on kahuna, metadata-editor (created by @ochiengolanga and merged by @sihil 8 minutes and 41 seconds ago) Please check your changes! |
Seen on media-api (created by @ochiengolanga and merged by @sihil 8 minutes and 51 seconds ago) Please check your changes! |
Seen on leases (created by @ochiengolanga and merged by @sihil 8 minutes and 56 seconds ago) Please check your changes! |
Seen on cropper (created by @ochiengolanga and merged by @sihil 9 minutes and 1 second ago) Please check your changes! |
Seen on image-loader (created by @ochiengolanga and merged by @sihil 9 minutes and 8 seconds ago) Please check your changes! |
What does this change?
It introduces a way of rendering additional fields on the UI via configuration. This is achieved by adding the following configuration to Kahuna service and Media API service configuration file i.e.
$USER/.grid/kahuna.conf
or/etc/grid/kahuna.conf
$USER/.grid/media-api.conf
or/etc/grid/media-api.conf
where applicable (dev stage or prod stage respectively).
The approach is described in the proposal: #3167
How can success be measured?
On the Image view page, the configured fields are visible on the “Show additional metadata” section when expanded.
Screenshots
Who should look at this?
@AWare , @sihil , @paperboyo and @akash1810
Tested?