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

Fedora bootstrap #1313

Merged
merged 1 commit into from Mar 14, 2017
Merged

Fedora bootstrap #1313

merged 1 commit into from Mar 14, 2017

Conversation

ryanlerch
Copy link
Collaborator

Here is a first pass at converting bodhi to fedora-bootstrap.

Most of the chrome is updated, and have done a major overhaul of the update page too.

would like to get this one merged, so we can finish it off by doing lots of little changes, since this is a pretty big PR already.

@bowlofeggs
Copy link
Contributor

The create update form seems to be cut off on the left side of my screen, in Firefox 51.0.1:

bodhi_screenshot1

I also noticed that the pagination links are vertically aligned instead of horizontally aligned on the search page:

bodhi_screenshot2

The JavaScript warning is being displayed for me even though I have JavaScript enabled on the create override view:

bodhi_screenshot3

% else:
Edit Update Form Requires JavaScript
Editin an update requires JavaScript
Copy link
Contributor

Choose a reason for hiding this comment

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

*Editing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed! thanks

<link href="https://apps.fedoraproject.org/global/fedora-bootstrap-1.0.1/fedora-bootstrap.css" rel="stylesheet" />
<link rel='stylesheet' href='//cdn.jsdelivr.net/font-hack/2.020/css/hack.min.css'>
<link href='https://fonts.googleapis.com/css?family=Open+Sans:400,300,300italic,400italic,700,700italic' rel='stylesheet' type='text/css'>
<link href="https://ryanlerch.fedorapeople.org/font-awesome.css" rel="stylesheet" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we merge this, I think we should try to get these css files packaged for Fedora, or at least document what they are (version/license/upstream) and put them in Bodhi's server/static folder so we don't depend on external services. Some of these things get blocked by Privacy Badger (like the googleapis.com link).

I don't mind taking on the packaging work if you don't want to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the other apps, we have been using the resources on apps.fp.org/global for these resources. I have updated these links to use these resources now.

One note, however, once i make this change to use the fontawesome from apps.fp.o, the fontawesome icons won't show up anymore when testing locally. This is because apps.fp.o has CORS restriuctions on non-fedora domains. I have submitted a patch to add the local fallback as a last resort in font-awesome.css, so it will work when testing locally if you install the font awesome fonts locally from the fedora repos.

@bowlofeggs
Copy link
Contributor

bowlofeggs commented Mar 2, 2017

I'm very excited about this change, it's a wonderful design. It's so much more usable - the information I want to see when viewing an update is right at the top. I compared an update view with how it looks in Bodhi and this is vastly superior presentation of the information. This is fantastic work Ryan!

There do seem to be a few failing unit tests. Let me know if you would like assistance with fixing these.

I was hesitant about merging it when it is partially done, but I think the whole app still works reasonably well and I think the unchanged parts still look nice enough, so I think I'm OK with merging this (once we fix those things I noted) as long as you would be OK with having a release in this state (I may make another Bodhi release soon). If you would rather not make a release in this state, I have a few thoughts:

Bodhi contributions don't often touch the html/css/js files (though as you can see this PR is currently conflicting which is contradicting me). If we kept this open in a feature branch, I don't think we'd experience too many conflicts. Furthermore, I can watch out for PRs that do change the html and make sure we handle the conflicts as they happen (which again, should be infrequent).

If you do want to merge as-is and are comfortable with a release in this state, can you also squash your commits into one?

Oh, and can you put this in your commit message, because I think you've fixed a bug while you've done this:

fixes #1216

@ryanlerch
Copy link
Collaborator Author

Thanks! Glad you like the changes!

I have done the following:

  • fixed the unit tests
  • fixed the way paginiation is displayed -- it is now horizontal again
  • fixed the create update form -- put it in a container, and ensured the form is not chopped off on the left anymore.
  • made the javascript warning on the new override page only show when JS is disabled.
  • fixed the "editin" typo
  • made the fonts link to the apps.fp.o/global sources as we do with other fedora-bootstrap based apps
  • squashed all this down into a single commit. :)

I am happy to release in this state, as there are no noticeable regresssions on other pages.

cheers,
ryanlerch

<link href="https://apps.fedoraproject.org/global/fedora-bootstrap-1.0.1/fedora-bootstrap.css" rel="stylesheet" type='text/css' />
<link href='https://apps.fedoraproject.org/global/fedora-bootstrap-fonts/open-sans.css' rel='stylesheet' type='text/css' />
<link href='https://apps.fedoraproject.org/global/fedora-bootstrap-fonts/hack.css' rel='stylesheet' type='text/css' />
<link href='https://apps.fedoraproject.org/global/fedora-bootstrap-fonts/font-awesome.css' rel='stylesheet' type='text/css' />
Copy link
Contributor

Choose a reason for hiding this comment

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

Fedora is actually not the only user of Bodhi. I've had a couple of pull requests from other users of Bodhi that had their own deployments. Due to the CORS issue you cited, and also due to us not knowing if all Bodhi deployments are on the public internet I don't think we should rely on this way of hosting these web assets.

How about we just bundle these assets into Bodhi for now, since that's how Bodhi has been doing this up until now? I can look into unbundling it later - even though bundling isn't ideal it's not a step backwards since Bodhi is alread doing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do have the font-awesome packages installed on my system but my browser doesn't seem to have all the fonts it needs (and I assume the CORS restriction is blocking it). Quite a few things are missing, like the search icon:

bodhi_screenshot1

@bowlofeggs
Copy link
Contributor

Thanks for the fixes! This does look pretty great now, and I don't notice any other issues outside of the web assets. What do you think about bundling those web assets for now? If you are OK with doing that I think this'll be good to merge.

@ryanlerch
Copy link
Collaborator Author

OKay! Just updated the PR to bundle the fonts and fedora-bootstrap.

The static dir is a bit cluttered, and needs to be reorganized and cleaned out, but i will do that in an additional PR.

@ryanlerch
Copy link
Collaborator Author

Just updated the commit to add icons back in on the index page (rss icons), and on the bugs and automated tests pages on updates. Also got the colours working again on the automated tests page.

Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Hey @ryanlerch!

I noticed one issue when trying out this PR again. If you go to an update and enter a comment the form clears when you click submit, but the comment does not appear on the page and there isn't any indication that the server accepted the comment. If you reload the page you can see that the comment was accepted. Should we have JS that automatically renders the new comment, or maybe just refresh the page when the submit button is pushed?

The next thing I have to say I feel bad about since I asked to bundle - I didn't realize the static content would be as large as it is. git can handle this, but if we check it in to git it will be in git forever (even if we later unbundle it), which means that git cloning bodhi will now be ~20 MB bigger from here on out if we merge. I did some reading on serving static content with Pyramid here:

http://docs.pylonsproject.org/projects/pyramid/en/latest/narr/assets.html#static-assets-section

We don't actually do that in production as we serve all static content with httpd instead of with Bodhi. I think we can organize the static content in such a way that allows us to have the dev environment and production both serve these files from RPMs. Are all these files available in RPMs? If so, I think we can do some tricks like this:

config.add_static_view(name='static/fonts/fontawesome', path='/usr/share/fonts/fontawesome')

That snippet would go into bodhi/server/__init__.py where a similar line is configured to serve static content today.

We could do similar for bootstrap and the css stuff. This would get the dev environment working, and I could do something analogous in Ansible for our production config.

Since I suggested that you do this work in the first place I'm willing to make that change for you if you want. I think GitHub has a way for me to push to your pull request nowadays, though maybe you have to allow that? Not sure, but let me know if you'd like me to take that on.

tl;dr; if you can fix the comment submission form I'm willing to take on the RPM-based font/js/css stuff.

@bowlofeggs
Copy link
Contributor

Also if this stuff isn't in RPMs I don't mind doing the packaging work to get it there.

@bowlofeggs
Copy link
Contributor

Actually, maybe it's not so bad:

$ du -sch bodhi/server/static/
13M     bodhi/server/static/
13M     total

vs. how it is without this PR:

$ du -sch bodhi/server/static/
5.4M    bodhi/server/static/
5.4M    total

It's really on the same scale as it is today, so perhaps I'm worrying about git history for nothing.

@bowlofeggs
Copy link
Contributor

A fresh git clone today is about 42 MB, so this would increase the git repo size by about 20%.

@bowlofeggs
Copy link
Contributor

Also, it would be good to add a note to the docs/release_notes.rst file for this change!

This commit changes the style of bodhi over to
the new fedora-bootstrap style that we are using
for Fedora Apps. The major changes are that the
chrome of the website has been updated to the new
style, and the page for each individual update has
also been refactored.

fixes #1216
@ryanlerch
Copy link
Collaborator Author

  • updated the release notes. I wasn't sure about the version scheme, so i made a 'develop' item instead of the version number
  • fixed the comments not updating on the update page. There was already javascript in there to do this, i just broke it by rearranging how the comments were structured. Fixed now though

@ryanlerch
Copy link
Collaborator Author

ryanlerch commented Mar 14, 2017

@bowlofeggs Are you happy with how the fonts are bundled right now? I haven't changed anything about it yet. Happy for you to jump in and change it if needs be.

Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the comment submission, it does seem to work now. It looks like the Hack font isn't packaged for Fedora yet so it wouldn't be trivial for me to unbundle the fonts for now. Though I don't like making the git repository bigger forever by adding these files, when I weigh it against keeping this pull request open for a long time to get the ideal case I've decided to go ahead and merge as-is. Bodhi already has a bunch of bundles assets, so we can consider this an item to solve later.

Thanks for the hard work, this is really a wonderful change!

@bowlofeggs bowlofeggs merged commit 98297a3 into fedora-infra:develop Mar 14, 2017
@bowlofeggs
Copy link
Contributor

I filed #1348 to track the unbundling effort.

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.

None yet

2 participants