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 #460

Open
wants to merge 4 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 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

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


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.

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

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
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 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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants