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.

Sign up for GitHub

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

Jump to bottom

Wilma Responsive redesign #460

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

Wilma Responsive redesign #460

wants to merge 4 commits into from

Conversation

milospp
Copy link
Contributor

@milospp milospp commented Apr 9, 2024

GitHub issue 3910
GitHub PR VIVO 3972

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?

Added bootstrap v5
Refactored nav tabs in Person, Org Unit page to use bootstrap component instead of jQuery
Refactored files locations

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 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 milospp mentioned this pull request Apr 9, 2024
Open
@chenejac chenejac requested a review from litvinovg April 10, 2024 06:22
@chenejac chenejac requested a review from balmas May 21, 2024 09:16
balmas
balmas reviewed May 22, 2024
View reviewed changes
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 code and responsiveness looks good to me but see my comment in vivo-project/VIVO#3972 about a problem loading the temporal google charts

webapp/src/main/webapp/js/individual/propertyGroupControls.js Outdated

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 print

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I removed it.

balmas
balmas approved these changes May 22, 2024
View reviewed changes
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 resolved in the merge, I think that this looks good

litvinovg
litvinovg reviewed May 24, 2024
View reviewed changes
Copy link
Contributor

@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.

I suggest to use webjar dependency #467
Also I think boostrap3 files shouldn't be moved as they are out of scope for this PR (Wilma didn't use boostrap3 before).

@milospp
Copy link
Contributor Author

milospp commented May 27, 2024

I suggest to use webjar dependency #467 Also I think boostrap3 files shouldn't be moved as they are out of scope for this PR (Wilma didn't use boostrap3 before).

I agree that Bootstrap 3 location refactor is not related to the Wilma theme directly. I can revert those changes, but would it be okay to have Bootstrap 5 on this location:
Vitro/webapp/src/main/webapp/bootstrap-5.3.2
and Bootstrap 3 on this:
Vitro/webapp/src/main/webapp/js/bootstrap

(Bootstrap 3 files are stored inside js subfolder)

@litvinovg
Copy link
Contributor

I suggest to use webjar dependency #467 Also I think boostrap3 files shouldn't be moved as they are out of scope for this PR (Wilma didn't use boostrap3 before).

I agree that Bootstrap 3 location refactor is not related to the Wilma theme directly. I can revert those changes, but would it be okay to have Bootstrap 5 on this location: Vitro/webapp/src/main/webapp/bootstrap-5.3.2 and Bootstrap 3 on this: Vitro/webapp/src/main/webapp/js/bootstrap

(Bootstrap 3 files are stored inside js subfolder)

With webjar there would be no need to upload bootstrap js/css assets into the project on each update and the path would /webjars/bootstrap/ (without version), which is important for customized themes update as js/css paths wouldn't change on next update.
Also all assets would be in one place, not like this one.

litvinovg and others added 4 commits May 31, 2024 13:37
@litvinovg
Added Boostrap 5 webjar dependency
9605af0
@milospp
Cherry-pick
e0bf28d 
Added Bootstrap and updated CSS for Wilma theme to make it responsive (vivo-project#429)

* Updated wilma to bootstrap 5

* Moved bootstrap to webapp folder, changed property tabs to use bootstrap

* Fixed property group tabs

* Bugfix: multiple dom object with same id, individual tabs

* Removed duplicate bootstrap 5.3.2

* Removed unused js code in propertGroupControls

* Reimplemented tab state persistence after refresh feature

* Removed unnecessary code
@milospp
Removed debug prints
34afe5a
@milospp
Removed bootstrap5 files from webapp
3e89331
@milospp milospp force-pushed the feature/wilma-improvement branch from 58c6d71 to 3e89331 Compare June 5, 2024 13:57
@balmas
Copy link

balmas commented Jun 10, 2024

@milospp is this one ready for re-review now?

@gneissone gneissone self-requested a review August 5, 2024 23:07
gneissone
gneissone approved these changes Aug 5, 2024
View reviewed changes
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 great, thank you.

@litvinovg litvinovg self-requested a review August 13, 2024 09:24
litvinovg
litvinovg requested changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@litvinovg litvinovg litvinovg requested changes

@gneissone gneissone gneissone approved these changes

@balmas balmas balmas approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@milospp @litvinovg @balmas @gneissone