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

Wilma Responsive redesign #3972

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

milospp
Copy link
Contributor

@milospp milospp commented Apr 9, 2024

GitHub issue 3910
GitHub PR Vitro 460

What does this pull request do?

Added bootstrap (only grid style) CSS and modified Wilma theme to leverage its responsive design feature and enhance the overall layout.

What's new?

Mobile optimized design
Avoided horizontal scroll

How should this be tested?

Open every page and test by resizing up to around 400px in width (Mobile device toolbar can be used in google chrome to simulate phone display)

Additional Notes:

Here is the video example

  • The Left window is VIVO with an old Wilma theme
  • The middle window is a new, updated Wilma theme
  • The right window is Tenderfoot theme (just for comparison)

Video Example
https://youtu.be/9B0_cevrEeE

milospp added 2 commits April 9, 2024 10:51
Added Bootstrap and updated CSS for Wilma theme to make it responsive (vivo-project#3909)

* Added bootstrap and updated CSS for Wilma theme to make it responsive

* Fixed View All position

* Fixed footer

* Added responsiveness to Page managment table

* Fixed CapabilityMap responsive

* modified Photo Cropping page and minor fixes

* Removed unused libraries

* Fixed horizonal scroll in the site admin pages

* Minor fix in Capability map page

* fix: Fixed serach bar button moving out of place

* fix: Co-author Network graph size on mobile phone

* Fixed tenderfoot bootstrap in search page

* Updated bootstrap location and refactored propert group tabs

* Reimplemented tab state persistence after refresh feature
@gneissone
Copy link
Member

@milospp took this for a test spin and works great overall. Note for future reviewers, it is necessary to do a hard refresh to clear your cache of the old wilma.css! I was fooled for a few minutes.

@chenejac chenejac requested a review from balmas May 21, 2024 09:16
Copy link

@balmas balmas left a comment

Choose a reason for hiding this comment

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

The responsiveness looks good to me. Tested in Chrome, FF and Safari.

However, although I cannot see any reason for this in the code, so it could be something caused by environment, I cannot load any of the temporal graph google charts when I build from the PR code. I get a cross-site scripting failure from the googlecharts API and jquery query error for the chart height. (see attached for example)
English-Temporal-Graph-Visualization (1)

I cannot understand why this is only the case in the newer code, but it is consistently reproducible for me. Apologies if this is something I'm doing wrong.


if ( groupName == "viewAll" ) {
processViewAllTab();
}
console.log("View all")
Copy link

Choose a reason for hiding this comment

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

suggest removing console debugging statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks for the suggestion.


<fieldset>
<legend>${i18n().search_form}</legend>
<form id="search-homepage" action="${urls.search}" name="search-home" role="search" method="post" >
Copy link

Choose a reason for hiding this comment

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

although I don't see any problem with this, just noting the switch from GET to post for the form submission. it doesn't seem related to the bootstrap theming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a while since I added this code, I can't remember why I switched to the POST method... Also, I see that every other theme is using GET.
I reverted it to GET.

@gneissone
Copy link
Member

@balmas the google chart issues are resolved via #3974, so hopefully @milospp can fix that by merging the main branches into the PRs to catch up without too many merge conflicts.

Copy link

@balmas balmas left a comment

Choose a reason for hiding this comment

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

Given that the chart issue is already fixed in main, and should be handled by the merge, I think this change looks good.

@@ -20,6 +20,8 @@
</#if>
</#if>

<script src="${urls.base}/bootstrap-5.3.2/js/bootstrap.min.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems bootstrap5 and bootstrap3 would be loaded at the same time according to head.ftl and headScripts.ftl of Nemo theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. It looks like I mistakenly added Bootstrap 5. I removed it from the Nemo theme and tested the themes. It's working fine now. If you catch anything strange, please report it.

@gneissone
Copy link
Member

Tried out the changes and looks good. @milospp, I did notice some odd behavior if the geo focus map was enabled in runtime.properties (homePage.geoFocusMaps=enabled). See screenshot:
Screenshot 2024-07-15 at 14 49 50

@milospp
Copy link
Contributor Author

milospp commented Aug 4, 2024

Tried out the changes and looks good. @milospp, I did notice some odd behavior if the geo focus map was enabled in runtime.properties (homePage.geoFocusMaps=enabled). See screenshot: Screenshot 2024-07-15 at 14 49 50

@gneissone, I fixed the map responsiveness. Can you please validate it.

image

@gneissone gneissone self-requested a review August 5, 2024 23:07
Copy link
Member

@gneissone gneissone left a comment

Choose a reason for hiding this comment

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

Looks good after the update, thank you!

@balmas
Copy link

balmas commented Aug 8, 2024

Just confirming that I agree this still looks good after the change to use the bootstrap webjar.

Copy link
Collaborator

@litvinovg litvinovg left a comment

Choose a reason for hiding this comment

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

This set of PRs not only modifies Wilma theme to have responsive design, but also introduces global dependency for shared components on specific version of Bootstrap which can't safely be done retrospectively.
So it breaks backward compatibility for customized themes
https://wiki.lyrasis.org/display/VIVODOC115x/Creating+a+custom+theme
I think we should find a better way to do that.

@chenejac
Copy link
Contributor

@milospp we concluded at a committers meeting the following:

  • This should be a new theme called willow
  • Changes of shared files in this PR (e.g. webapp/src/main/webapp/css/*) should be moved under the theme directory if it is possible

Can you please align this PR in accordance with those conclusions? Thanks

@gneissone
Copy link
Member

@chenejac is it also reasonable to change the default theme to 'willow' as part of the PR?

@chenejac
Copy link
Contributor

@chenejac is it also reasonable to change the default theme to 'willow' as part of the PR?

General idea is that willow will be the default VIVO theme. The team will make the final decision when this PR is close to be merged.

@hauschke
Copy link
Member

Just a quick note regarding the name: AFAIK, Wilma and Tenderfoot were named after... "Wilma Tenderfoot". In that tradition it could make sense to call it "Pickle", "Bludsten", or "Theodore". But that just as a sidenote, I have no strong feelings here.

@brianjlowe
Copy link
Member

Wilma was named after the first granddaughter of Jon Corson-Rikert, the IT director at Mann Library and original creator of VIVO.

@hauschke
Copy link
Member

Wilma was named after the first granddaughter of Jon Corson-Rikert, the IT director at Mann Library and original creator of VIVO.

Ah, that's cute and new to me. Okay, so (Wilma) Tenderfoot has nothing to to with the original Wilma. Thanks for the correction! But I am sure that Tenderfoot was named after Wilma Tenderfoot.

@brianjlowe
Copy link
Member

I had no idea where Tenderfoot came from, but that certainly makes sense if someone reinterpreted Wilma in a new context.

@gneissone
Copy link
Member

gneissone commented Nov 27, 2024

re:#3972 (review)

I am still seeing the issue with the temporal graphs after merging in the main branch, unfortunately. The sparkline graph works fine. @milospp any ideas what could be going wrong? I tried manually setting a width for the div holding the graph but there was no change. FWIW if I change the theme the temporal graphs work fine.

@gneissone gneissone self-requested a review November 27, 2024 22:26
Copy link
Member

@gneissone gneissone left a comment

Choose a reason for hiding this comment

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

Temporal graph issue needs resolved.

}
}

#graphContainer {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is the problem. If this #graphContainer css is removed then the temporal graph works again.

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.

7 participants