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

Add thumbnail for featured dataverses #10433

Merged

Conversation

jeromeroucou
Copy link
Contributor

@jeromeroucou jeromeroucou commented Mar 27, 2024

What this PR does / why we need it:

With this PR, a dataverse administrator can set a thumbnail image into theme page, to be displayed in featured dataverses. Unlike logo or footer images, the preview image has no display parameters such as "logo format" or "logo alignment".

The following rule applies to the images displayed for featured collections: if a preview image is present then it's displayed, else if a banner is present then it's displayed, else the default Dataverse logo is displayed.

Which issue(s) this PR closes:

Special notes for your reviewer:

I have to refactor some code to fix a bug : I move image suppression block instruction from UpdateDataverseThemeCommand.java to ThemeWidgetFragment.java. When the logo was first deleted, the theme was saved in database with partial information (no footer). When the image footer was deleted, as the database theme no longer contained this information, so the image was not deleted.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

dataverse_thumbnail_featured_dataverses.webm

Additional documentation:

Dataverse management documentation page have been modify to add the feature

Preview docs at https://dataverse-guide--10433.org.readthedocs.build/en/10433/user/dataverse-management.html#theme

@coveralls
Copy link

coveralls commented Mar 27, 2024

Coverage Status

coverage: 20.754% (-0.005%) from 20.759%
when pulling 27f682f on Recherche-Data-Gouv:10291-collection_thumbnail
into d4b9260 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Interesting feature! I left some quick feedback. Thanks for the video!

@@ -69,7 +69,8 @@ The Theme features provides you with a way to customize the look of your Dataver

- Inherit the theme from the parent Dataverse collection. This option is helpful if you'd like consistency across several Dataverse collections that all share the same parent.
- Add or update a logo image, which will appear at the top of your Dataverse collection.
- Add or update a footer image, which will appear at at the bottom of your Dataverse collection.
- Add or update a thumbnail image, which will appear on featured dataverses of your Dataverse collection.
Copy link
Member

Choose a reason for hiding this comment

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

Interesting feature! My first thought is, should this feature be available via API? That way the new frontend can use it some day.

Also, these issue are related:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to have thumbnail logo via API, all Theme feature have to be moved before. Do you know if the development of #10194 is well advanced and PR will be open soon ?

Either I wait for PR #10194 to be accepted (and I adapt the source code from it), or I propose one. But either way, I should change the status of this PR (#10433) to draft.

Copy link
Member

Choose a reason for hiding this comment

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

I got the impression from @DS-INRA that he might have a team member like you work on #10194 but maybe I'm wrong! 😅

Copy link
Member

Choose a reason for hiding this comment

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

Please be aware that we are changing how we name these flyway files:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I hadn't realized this change, thanks for the information !

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Just a suggestion for the release note.

doc/release-notes/10433-release-notes.md Outdated Show resolved Hide resolved
@cmbz
Copy link

cmbz commented Mar 28, 2024

2024/03/28

jeromeroucou and others added 3 commits March 29, 2024 17:34
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
Add new recommandations (HTML preview + "for more information ...")
@cmbz cmbz added the Size: 10 A percentage of a sprint. 7 hours. label Apr 10, 2024
@pdurbin
Copy link
Member

pdurbin commented Aug 5, 2024

@jeromeroucou this PR has merge conflicts. Can you please resolve them?

@pdurbin pdurbin added the Status: Needs Input Applied to issues in need of input from someone currently unavailable label Aug 5, 2024
@DS-INRA
Copy link
Member

DS-INRA commented Aug 7, 2024

Hi @pdurbin , @jeromeroucou is currently on vacation and will be able to get back to this PR in september :)

@pdurbin
Copy link
Member

pdurbin commented Aug 27, 2024

Maybe @luddaniel can fix the merge conflicts? They seem pretty straightforward but I can't push to https://github.com/Recherche-Data-Gouv/dataverse

It should be just a matter of renaming the sql file (bumping the version to the next available):

pdurbin@beamish dataverse % git merge develop
Auto-merging src/main/java/edu/harvard/iq/dataverse/DataverseServiceBean.java
Auto-merging src/main/java/propertyFiles/Bundle.properties
Auto-merging src/main/resources/db/migration/V6.2.0.1.sql
CONFLICT (add/add): Merge conflict in src/main/resources/db/migration/V6.2.0.1.sql
Automatic merge failed; fix conflicts and then commit the result.
pdurbin@beamish dataverse % 

Heads up that there's also a sql file in #10694 so one of these PRs will need to bump again. Here are our docs on the matter, if you haven't seen them: https://guides.dataverse.org/en/6.3/developers/sql-upgrade-scripts.html

@pdurbin pdurbin self-assigned this Aug 27, 2024
@luddaniel
Copy link
Contributor

luddaniel commented Aug 28, 2024

@pdurbin done ;) we will rename sql file once again if needed.

By the way, RemoteOverlayAccessIOTest.testRemoteOverlayFiles:103 expected: <true> but was: <false> is also failing on develop

@cmbz cmbz added FY25 Sprint 4 FY25 Sprint 4 FY25 Sprint 5 FY25 sprint 5 labels Aug 28, 2024
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I had a quick play with this and it seems to work. The code seems fine, I think. Approved.

@pdurbin
Copy link
Member

pdurbin commented Sep 4, 2024

Screenshots from playing around with this feature:

Screenshot 2024-09-04 at 11 44 01 AM
Screenshot 2024-09-04 at 11 43 35 AM
Screenshot 2024-09-04 at 11 43 23 AM

@pdurbin pdurbin removed their assignment Sep 4, 2024
@pdurbin pdurbin removed the Status: Needs Input Applied to issues in need of input from someone currently unavailable label Sep 4, 2024
@stevenwinship stevenwinship self-assigned this Sep 4, 2024
@stevenwinship stevenwinship merged commit a8016aa into IQSS:develop Sep 4, 2024
14 checks passed
@stevenwinship stevenwinship removed their assignment Sep 4, 2024
@pdurbin pdurbin added this to the 6.4 milestone Sep 4, 2024
@jeromeroucou jeromeroucou deleted the 10291-collection_thumbnail branch September 5, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 4 FY25 Sprint 4 FY25 Sprint 5 FY25 sprint 5 Size: 10 A percentage of a sprint. 7 hours.
Projects
Status: Done 🧹
Status: Done
Development

Successfully merging this pull request may close these issues.

Dataverse collection thumbnail in addition to Logo Image as banner
7 participants