New frontend HTML/CSS theme #160
Conversation
|
How is it supposed to be installed for development and production? There are absolute paths to your filesystem, so I'm not sure how I should test it |
|
These are not absolute paths, but URLs relative to deployment URL: https://apps.fedoraproject.org/ |
|
Thanks. And what is the plan for production? Would the stylesheets be packaged as RPMs? |
|
For production in Fedora infrastructure, we would use resources already available on apps.fedoraproject.org, like https://apps.stg.fedoraproject.org/global/fedora-bootstrap-1.0.3/fedora-bootstrap.css |
|
Would it work with unbranded bootstrap? |
|
Yes. |
|
Then what about putting the URL set in configuration and leaving the decision for the admin? Fedora deployments would use apps.fedoraproject, non-fedora deployments could use some of the public CDNs or host their own copy |
|
Sure. I'll do that. |
| if encoded: | ||
| return '?' + encoded | ||
| return '' | ||
|
|
There was a problem hiding this comment.
Could you please keep this if? It looks ugly when all URLs now have a trailing ?. People wll be copying those to emails and IRC
There was a problem hiding this comment.
This is not just cosmetic, but there is technical reason - URL "?" is valid and means "current page with empty query args". Alternative would be specifying full relative URL. But if you insist I can work on that.
templates/macros.html
Outdated
| <div class="float-right"> | ||
| <a class="btn btn-secondary btn-sm" href="{{ url_for('build_detail', build_id=build.id) }}" | ||
| data-toggle="tooltip" data-placement="right" | ||
| title="Show details of this build">...</a> |
There was a problem hiding this comment.
This could be more intuitive with an icon like fa-info or fa-info-circle
There was a problem hiding this comment.
I've changed it to fa-info, but for me ellipsis was better.
templates/package-detail.html
Outdated
| </div> | ||
|
|
||
| {% for (name, url) in generate_links(package) %} | ||
| <a class="card-link" href="{{ url }}">{{ name }}</a> |
There was a problem hiding this comment.
Can you put less links per row? There's a lot of space underneath, I'd suggest just 2 columns
There was a problem hiding this comment.
I've made it into 2 columns as suggested.
templates/package-detail.html
Outdated
| {% if items|length > max %} | ||
| <div class="row"> | ||
| <div class="col-sm-2 offset-5"> | ||
| <a class="btn btn-secondary btn-sm kk-toggle-button" |
There was a problem hiding this comment.
Can you put some margin around this button or below the change list? It sometimes touches the descenders of the last line.
There was a problem hiding this comment.
I've added "mt-2" margin.
static/koschei.css
Outdated
| @@ -24,183 +24,6 @@ | |||
| Michael Simacek <msimacek@redhat.com> | |||
| */ | |||
|
|
|||
| html { | |||
| min-width: 800px; | |||
| } | |||
There was a problem hiding this comment.
Can you please add the following, so that the font sizing matches other Fedora apps that use bootstrap-fedora?
html {
font-size: 14px;
}
templates/group-detail.html
Outdated
| <dd class="col-sm-9"> | ||
| <ul class="list-inline"> | ||
| {% for owner in group.owners %} | ||
| <li class="list-inline-item">{{ owner.name }}</li> |
There was a problem hiding this comment.
Can you please join them with commas? Also the spacing looks a bit odd, maybe it would look better without columns
| </li> | ||
| {% endfor %} | ||
| </ul> | ||
| </div> |
There was a problem hiding this comment.
The collection chooser was quite ugly, but it was there for a reason. When you click a package link in a multicollection view (like group packages or user packages), you always get package detail for the first collection (in fedora it's rawhide). So a user goes to listing of his packages, he sees that foo is broken in epel7, clicks on it and gets confused, because he sees that the builds are ok, because he is looking at rawhide, even though he wanted epel7. This gets much worse for deployments that have sparse collections (there are many collections, but packages usually exist in just one or two of them) - you click on a package link and you're almost always greeted with a message that the package doesn't exist in selected collection.
There was a problem hiding this comment.
And with the current tabs, if you didn't select a collection, none of them is displayed as active, so you don't know which collection you're looking at.
There was a problem hiding this comment.
When you click a package link in a multicollection view (like group packages or user packages), you always get package detail for the first collection (in fedora it's rawhide).
The same happens in current master too (without this PR).
Do we want to fix it? I.e not show any builds unless explicit collection is choosen.
There was a problem hiding this comment.
The same happens in current master too (without this PR).
That's true. I'm wondering when it regressed.
Do we want to fix it? I.e not show any builds unless explicit collection is choosen.
It was quite important in deployments that didn't have a "master" collection like Fedora has. Since we currently don't maintain such deployments, I think we can keep the current behavior until we need more flexibility again.
mizdebsk
force-pushed
the
mizdebsk/fedora-bootstrap-theme
branch
2 times, most recently
from
a50f508
to
a867737
Compare
mizdebsk
changed the title
mizdebsk
force-pushed
the
mizdebsk/fedora-bootstrap-theme
branch
from
a867737
to
15e2eeb
Compare
|
@msimacek This should be reviewable. I think I've addressed all your concerns or commented on them.
I've used some components specific to fedora-bootstrap (masthead, subheader), so this won't work with pure bootstrap any longer. |
|
Thanks! |
Reworked web interface - mostly complete, but still work in progress. Deployed in staging.
Changes include: