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

Templates revision - part 3 #4629

Merged
merged 17 commits into from
Jun 30, 2018
Merged

Templates revision - part 3 #4629

merged 17 commits into from
Jun 30, 2018

Conversation

Gwenwyfar
Copy link
Contributor

Let's get this out before it grows into a little monster. There aren't many things left that will involve editing the templates from here onwards.

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
.
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
+ some minor clean up. Appearance was changed little, just made to look more consistent.

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
This doesn't break any existing icons, but allows us to remove any unecessary spans and makes the icons more versatile.

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
+ add some space between elements

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
+ small cleanup on boards code to remove redundant css. windowbg2 no longer exists.

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Replaces inline styles with the new generic_bar class.

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
+ more lateral space for the preview

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Fixes #4577

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
+ more space for the paths

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
No more error log spam!

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
@equinoxmatt
Copy link
Contributor

Been looking at this for a short while tonight. I am not the best person to comment, but I took a look at your commits individually and checked your branch out. It seems fine, I can't see any issues.

I won't 'approve' it, because this isn't my area of expertise, but LTGM 👍

@wintstar
Copy link

@Gwenwyfar, the text in the reports is somewhat small. Is there a way to make this a little bigger?
index.php?action=admin;area=reports

admin.css line 31

.table_caption, tr.table_caption td {
    color: #000;
    font-size: 10px;
    font-weight: bold;
}

@Gwenwyfar
Copy link
Contributor Author

This PR is already long enough, but I'll add font size revision to one of my next ones. There are some oddly sized fonts elsewhere as well.

Copy link
Member

@Sesquipedalian Sesquipedalian left a comment

Choose a reason for hiding this comment

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

Mostly just some changes regarding hyphenation and word wrapping.

@@ -265,10 +265,6 @@ span.hidelink {
-ms-hyphens: auto;
hyphens: auto;
overflow-wrap: break-word;
word-wrap: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

Apparently, IE and Edge < 18 don't support the standard overflow-wrap, so we need to keep word-wrap: break-word; for them.
https://caniuse.com/#search=overflow-wrap

@@ -205,13 +205,14 @@ body#help_popup {
margin: 0 2px;
padding: 2px 4px;
font-size: 9px;
font-family: verdana, sans-serif; /* @todo Might add extra fallbacks. Possibly monospace. */
font-family: verdana, monospace, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

This change doesn't make a lot of sense to me. If we define a generic monospace before the generic sans-serif, then the latter will never be used. So I think we only need one, right? Personally, I'd stick with sans-serif rather than monospace for this.

@@ -2234,8 +2314,9 @@ dl {
box-shadow: 0 1px 0 #fff inset;
padding-top: 12px;
clear: both;
word-break: hyphenate;
word-wrap: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

Because support for hyphens is dependent not only on browser, but also on language, we should keep word-wrap: break-word; overflow-wrap: break-word; as a fallback.

width: 160px;
word-break: hyphenate;
word-wrap: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

As noted above, because support for hyphens is dependent not only on browser, but also on language, we should keep word-wrap: break-word; overflow-wrap: break-word; as a fallback.

@@ -3227,7 +3275,7 @@ div#pollmoderation {
border-top: 1px solid #bfbfbf;
box-shadow: 0 1px 0 #fff inset;
min-height: 85px;
word-break: hyphenate;
word-break: break-all;
Copy link
Member

Choose a reason for hiding this comment

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

word-break: break-all; is not very nice to readers. It'd be better to use hyphens: auto; (with the vendor prefixes, of course).

Copy link
Contributor Author

@Gwenwyfar Gwenwyfar Jun 19, 2018

Choose a reason for hiding this comment

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

Hyphens will not break some links and other overly long words. And unfortunately text-ellipsis needs white-space: nowrap which we don't want here either. And outside those cases there is enough space in this area that hyphens is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

If I can't convince you to use hyphens: auto, please at the very least use word-wrap: break-word instead of word-break: break-all. Using word-wrap: break-word will only break the word if absolutely necessary, whereas word-break: break-all will break any and all words whenever they reach the edge of the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I missed this.

@Sesquipedalian
Copy link
Member

Sesquipedalian commented Apr 25, 2018

Apparently GitHub doesn't allow comments on just any random line, so I couldn't add one for #basicinfo h4 on line 2186 of index.css. That selector needs to have the following added to its ruleset in order to fix #4719:

word-wrap: break-word;
overflow-wrap: break-word;
-webkit-hyphens: auto;
-ms-hyphens: auto;
hyphens: auto;

@Sesquipedalian
Copy link
Member

I just noticed some other spots where overflow-wrap is being used without word-wrap being set as as compatibility fallback for IE. That should be fixed everywhere.

@Gwenwyfar
Copy link
Contributor Author

Just to update on this, I'm hoping to make the fixes soon, as I'm finally getting some free time back :)

@XinYenFon
Copy link
Contributor

I think at this stage its better to just remove the conflict and merge this big one in and do rest of the fixes later.

@Gwenwyfar
Copy link
Contributor Author

Gwenwyfar commented Jun 19, 2018

Nevermind, got the quirks to replicate once more. We may need to watch out those places to see if they aren't breaking in strange ways, still. Those rules are still unreliable.

Fixes #4719

Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
@Gwenwyfar
Copy link
Contributor Author

Can you handle the conflicts, Sesq? I could do it but it's going to also merge the PR (if there's another way through GitKraken I haven't it figured out yet)

@Sesquipedalian Sesquipedalian merged commit 81d6421 into SimpleMachines:release-2.1 Jun 30, 2018
@Gwenwyfar Gwenwyfar deleted the css+templating branch July 1, 2018 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants