GNOME Bugzilla – Bug 668209
theme - harmonise text sizes and weights
Last modified: 2012-02-29 11:25:39 UTC
The theme contains a large number of different text styles. They are a bit of a mess right now, with a mixture of sizes sometimes defined using different units (both em and pt are used). It would be great if we could harmonise the text styles by reducing their number and using the same units. It would also help if we could define them either as shared properties in the CSS or by documenting them somewhere.
Created attachment 207845 [details] [review] initial patch An initial patch which sets a single shared text style. It makes the app launcher labels a bit bigger in the process.
Review of attachment 207845 [details] [review]: Sure.
I've experimented with setting shared text styles for the rest of the theme. This has convinced me of two things: 1. The theme is *really* in need of a cleanup 2. Trying to clean it up using straight css is almost impossible. I think we need something like scss.
(In reply to comment #3) > I've experimented with setting shared text styles for the rest of the theme. > This has convinced me of two things: > > 1. The theme is *really* in need of a cleanup > > 2. Trying to clean it up using straight css is almost impossible. > > I think we need something like scss. We chose CSS because it was better than hardcoding color values in the code. I don't know why CSS isn't good enough (some examples would be nice), but if it isn't, we own the stack, we can change it.
Gtk css allows sheets to be imported and named styles to be defined. Is this something that the shell can do too?
(In reply to comment #5) > Gtk css allows sheets to be imported and named styles to be defined. Is this > something that the shell can do too? Yes and no (@import is supposed to work, we don't have @define-color (atm))
Created attachment 208163 [details] [review] another patch Here's a slighly updated patch. I did do a bunch of work on this but hit some *ahem* issues. One thing is certain - this patch only scratches the surface. It would make it easier to clean up the theme if we could set the default text style in the css. Would that be possible?
(In reply to comment #7) > It would make it easier to clean up the theme if we could set the default text > style in the css. Would that be possible? Yes - text properties are inherited, so you can use stage {} (it's already used for the font-family, there's no reason why it shouldn't work for other properties like size, weight, etc.)
Created attachment 208321 [details] [review] patch - set default text style
Created attachment 208322 [details] [review] path - theme tidying
Created attachment 208323 [details] [review] patch - group contact theme elements together
Created attachment 208324 [details] [review] patch - update modal dialog theming
Created attachment 208325 [details] [review] patch - further text style cleanup
These five patches are my work so far. They define some shared text styles, including a default style. They also do a small amount of restructuring and add some more comments to break up the theme into sections. There's still plenty to do to clean up the theme, but this feels like a good initial step. The only user visible changes are very minor alterations in the size of the text. I've also tweaked the padding and colors used in modal dialogs so that they match the mockups. It would be worth trying to land this for 3.4, in my opinion. I haven't been able to test how these patches affect the various authorisation dialogs or the end session 'you have apps running' dialog.
Review of attachment 208321 [details] [review]: Two changes look like they don't belong in this patch, otherwise looks good. ::: data/theme/gnome-shell.css @@ +1665,2 @@ .run-dialog-entry { + padding-top: 4px; Unrelated change. @@ +1710,2 @@ padding-left: 17px; + padding-bottom: 60px; Dito.
Review of attachment 208322 [details] [review]: There's certainly more here than "updating some comments" - not to mention that some of the removals looks plain wrong. Also, the patch does not apply to master ... ::: data/theme/gnome-shell.css @@ +282,2 @@ .modal-dialog-button { + font-weight: bold; Unrelated change @@ -269,2 @@ .modal-dialog-button { - color: white; Should have been removed in the previous patch @@ -789,3 @@ - text-shadow: black 0px 2px 2px; - background-image: url("running-indicator.svg"); - background-size: contain; Really? In any case, unrelated to this patch. @@ -803,3 @@ - background-color: rgba(255,255,255,0.1); - text-shadow: black 0px 2px 2px; - transition-duration: 100; Dito. @@ -813,3 @@ -.app-well-menu { - font-size: 9pt; Unrelated change (and: do we really want to use 11pt here?) @@ -976,3 @@ .calendar-month-label { color: #666666; - font-size: 7.5pt; Again, looks unrelated.
Review of attachment 208323 [details] [review]: Moving the contacts stuff looks fine, but the unrelated changes will have to be moved to the patches they belong to. ::: data/theme/gnome-shell.css @@ -760,3 @@ - font-size: 8pt; - font-weight: bold; - color: white; Unrelated - should be moved into the first patch I guess. @@ +756,3 @@ } +.app-well-app.running > .overview-icon { Ha! So that's where all this stuff went - that part should be in the previous patch.
Review of attachment 208324 [details] [review]: The patch looks good to me (though I didn't actually try it); technically this is a UI break, although the changes are minor. I don't think anyone would mind (or honestly: even notice) if the patch is just committed, but check with Owen/Matthias anyway whether you should ask for a freeze break.
Review of attachment 208325 [details] [review]: The changes look OK, but I'd prefer the comment changes to go in the "theme tidying" patch.
Created attachment 208401 [details] [review] patch - set default text style
Created attachment 208402 [details] [review] patch - add sections and cleanup
Created attachment 208403 [details] [review] patch - move contacts theming to its own section
Created attachment 208404 [details] [review] patch - create a common small text style
Created attachment 208405 [details] [review] theme - modal dialog updates
These new patches are better organised. There are a couple of issues with the modal dialog patch: Increasing the padding on modal-dialog-button-box ends up adding space to the run dialog, even though it doesn't have any buttons. I'm not sure what the best thing to do there is - the dialog *should* probably have a close button (since there's no obvious way to exit if you accidentally trigger it). Also, I have been unable to set the color for all the subject icons that are used in the modal dialogs (so that the password dialog icon is white when it should be grey). It would be good to have a single style class for these icons. There are also a few dialogs that I haven't been able to test this patch against (mount operation, login).
Created attachment 208410 [details] [review] patch - move contact theming to its own section Fix a mistake in the 3rd patch.
Created attachment 208576 [details] [review] patch 1 - set default text style
Created attachment 208577 [details] [review] patch 2 - add sections and cleanup
Created attachment 208578 [details] [review] patch 3 - move contacts to its own section
Created attachment 208579 [details] [review] patch 4 - set a common small text style
Created attachment 208580 [details] [review] patch 5 - modal dialog updates
Review of attachment 208576 [details] [review]: You're changing a lot of 10.5pt stuff to 11pt. I'm not sure how big of a visual difference there is, but we're technically past the freeze, aren't we? If there's no noticeable difference, go ahead.
Review of attachment 208577 [details] [review]: ::: data/theme/gnome-shell.css @@ +670,3 @@ * padding compensates for this and ensures that the label is vertically * centered */ + We usually don't have whitespace after comments like this one. @@ +822,3 @@ } + Why the double space here?
Review of attachment 208578 [details] [review]: Sure.
Review of attachment 208579 [details] [review]: ::: data/theme/gnome-shell.css @@ -1944,2 @@ .prompt-dialog-error-label { - font-size: 10pt; You didn't invent a new style for these, meaning that they'll get the now default 11pt instead.
Review of attachment 208580 [details] [review]: How much does this break "The Freeze"?
(In reply to comment #32) > Review of attachment 208576 [details] [review]: > > You're changing a lot of 10.5pt stuff to 11pt. I'm not sure how big of a visual > difference there is, but we're technically past the freeze, aren't we? If > there's no noticeable difference, go ahead. This is mainly to clean up the theme. The visual changes won't be noticed. (In reply to comment #35) > Review of attachment 208579 [details] [review]: > > ::: data/theme/gnome-shell.css > @@ -1944,2 @@ > .prompt-dialog-error-label { > - font-size: 10pt; > > You didn't invent a new style for these, meaning that they'll get the now > default 11pt instead. Yep, that's intentional. (Again for cleanup purposes.) (In reply to comment #36) > Review of attachment 208580 [details] [review]: > > How much does this break "The Freeze"? The main difference is the padding, so the dialogs will be a different shape. don't see it being too big an issue. It might be worth checking though. The problem with this final patch is what it does to the run dialog - it ends up being a bit strange. I think we'll need some other changes before it can be landed.
Created attachment 208590 [details] [review] patch 1 - set default text style
Created attachment 208591 [details] [review] patch 2 - add sections and cleanup
Created attachment 208592 [details] [review] patch 3 - move contacts to its own section
Created attachment 208593 [details] [review] patch 4 - set a common small text style
Created attachment 208594 [details] [review] patch 5 - modal dialog updates
Some of the previous patches didn't apply, so here are new versions. Again, I think the last one needs more discussion before it's merged.
Created attachment 208607 [details] [review] modalDialog: Hide button layout by default For modal dialogs without buttons, the button group still contributes padding/spacing. To fix that, hide it by default and only show it when actually adding buttons. (In reply to comment #37) > The main difference is the padding, so the dialogs will be a different shape. > don't see it being too big an issue. It might be worth checking though. > > The problem with this final patch is what it does to the run dialog - it ends > up being a bit strange. I think we'll need some other changes before it can be > landed. This patch should fix the issue with the run-dialog ...
Review of attachment 208590 [details] [review]: LGTM
Review of attachment 208591 [details] [review]: Sure.
Review of attachment 208592 [details] [review]: Looks OK (it also moves the app view stuff, so "group contacts and application theming together" might be a more correct commit message, but I don't really care ;-)
Review of attachment 208607 [details] [review]: ::: js/ui/modalDialog.js @@ +94,3 @@ y_align: St.Align.END }); + // only show button group if buttons are added (via setButtons()) + this._buttonLayout.hide(); show/hide are going to be deprecated soon, if I remember correctly. Use visible: false in the props hash above as a replacement. @@ +114,3 @@ + this._buttonLayout.show(); + else + this._buttonLayout.hide(); this._buttonLayout.visible = buttons.length > 0;
Review of attachment 208593 [details] [review]: Minor nitpick, otherwise looks good (I'll leave it to you if you want to practice your rebase skills or just go ahead ;-) ::: data/theme/gnome-shell.css @@ -680,3 @@ padding: 4px 12px; background-color: rgba(0,0,0,0.5); - color: #ffffff; That line looks like it belongs in the first patch.
Created attachment 208613 [details] [review] modalDialog: Hide button layout by default Hmm, wasn't aware of show/hide going away, but sure ...
Review of attachment 208613 [details] [review]: Looks good.
Review of attachment 208594 [details] [review]: I'm happy if you are - technically this is a freeze break, so best ask on IRC if you should file a request or just go ahead ...
Comment on attachment 208613 [details] [review] modalDialog: Hide button layout by default Attachment 208613 [details] pushed as 760da64 - modalDialog: Hide button layout by default
I've pushed everything except for the modal dialog patch. Bug 670227 seems like a better fit for that so I'll continue with it there.
Created attachment 208661 [details] [review] patch - clean up modal dialog text styles The patch that I punted to bug 670227 actually had a few changes that are required for the work here.
Review of attachment 208661 [details] [review]: Sure, go ahead
Thanks Florian.