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

Upgrade flask-bootstrap to bootstrap-flask #41

Closed
wants to merge 5 commits into from

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Dec 30, 2020

This PR does the following:

  1. switches the outdated/not maintained flask-bootstrap package to bootstrap-flask. This updates from Bootstrap 3 to Bootstrap 4, so some of the elements needed to be renamed as well as modifications to the base package. Unfortunately, this leaves the diff on the HTML quite large, but all of the text and functionality is exactly the same.

  2. This PR also makes the web application requirements optional in case somebody doesn't need web functionality or is worried about conflicting flask-bootstrap and bootstrap-flask. (These changes were moved to Make flask requirement optional #43)

  3. Adds an entrypoint so the web app can be run simply with gilda from the command line (These changes were moved to Add Gilda CLI and entrypoints #42)

The two pages now look like this:

Home

Screen Shot 2020-12-30 at 23 43 32

Results

Screen Shot 2020-12-30 at 23 43 41

@bgyori
Copy link
Member

bgyori commented Jan 3, 2021

Thanks! The existing web app has worked quite well though and I liked the look. I also wasn't planning to make the webapp installs optional, it was my intention to just have it in the install and have it available by default. I'll try to get back to this and adjust the appearance at some point.

@cthoyt
Copy link
Member Author

cthoyt commented Jan 4, 2021

Sorry I have a bad habit of including several things in the same PR. I think it might be good to split up these three things into different PR's then evaluate separately. Having the flask installation be separate would definitely be good for me because I'm getting conflicts between bootstrap-flask and flask-bootstrap on the Biomappings code that imports gilda.

The entrypoints thing is cool, but totally cosmetic and the python -m gilda.app still continues to work. That's pretty irrelevant for the change in requirements

@cthoyt
Copy link
Member Author

cthoyt commented Jan 4, 2021

@bgyori please check #42 and #43 before coming back to this - I split out some of the other changes.

@cthoyt cthoyt changed the title Upgrade flask-bootstrap Upgrade flask-bootstrap to bootstrap-flask Jan 4, 2021
@bgyori
Copy link
Member

bgyori commented Jan 20, 2021

I'm still a bit conflicted about this because we have several other services that use flask-bootstrap that are running in the same Docker container or virtualenv as Gilda and need to use Gilda as a dependency. Given that flask-bootstrap and bootstrap-flask are incompatible, it would be a bunch of work to review all those builds and deployment environments and test that they work, etc.

@bgyori
Copy link
Member

bgyori commented Jan 20, 2021

Also in relation to #43, I wonder if there is a way to separate the original JSON REST API (which should be available in every default install) from the form-based GUI (which should be running on the deployed grounding.indra.bio but doesn't necessarily need to be installed when gilda is used programmatically). Then switching over to bootstrap-flask and making it an optional install would be less of a problem. Just not sure yet what a clean solution to this would be in terms of refactoring app.py.

@cthoyt
Copy link
Member Author

cthoyt commented Jan 20, 2021

Also in relation to #43, I wonder if there is a way to separate the original JSON REST API (which should be available in every default install) from the form-based GUI (which should be running on the deployed grounding.indra.bio but doesn't necessarily need to be installed when gilda is used programmatically). Then switching over to bootstrap-flask and making it an optional install would be less of a problem. Just not sure yet what a clean solution to this would be in terms of refactoring app.py.

I think this could be accomplished by using Blueprints and wrapping them in functions to hide imports. That could be a separate PR - let me know if you want me to give it a shot

@bgyori
Copy link
Member

bgyori commented Sep 4, 2021

For the time being I will close this because other related apps depend on flask-bootstrap, but we can revisit it later.

@bgyori bgyori closed this Sep 4, 2021
@cthoyt cthoyt deleted the upgrade-bootstrap branch January 22, 2025 09:41
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.

2 participants