-
Notifications
You must be signed in to change notification settings - Fork 2
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
improve display on mobile phones #30
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some markup problems; dn
inside dfn
.
There are lots of changes that appear to be formatting. They should be separated out into a separate PR. |
What does this actually do? |
spec/index.html
Outdated
caption { font-weight: bold; text-align: left ; } | ||
code {color: #ff4500;} /* Old W3C Style */ | ||
code { color: #ff4500; } /* Old W3C Style */ | ||
table { word-break: break-all; max-width: 80vw; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
table { word-break: break-all; max-width: 80vw; } | |
table { word-break: break-word; max-width: 80vw; } |
@pfps — I see only CSS changes — whitespace on several lines (looks like no effect on the stylesheet), and addition of one line (99): table { word-break: break-all; max-width: 80vw; } I think that should be @gkellogg — How are you seeing those " |
@gkellogg's concerns came from the Echidna Auto-publish failure report, to wit:
Now, those errors appear to be about lines 1535 and 1536 —
— which clearly do not contain the tags flagged by Echidna. Hopefully, once my change request is merged, which will re-trigger Echidna and PR-Preview, the issues will magically resolve, or the errors will be revised and point to lines that actually contain problematic tags. |
These may all be the result of malformed XML around line 202. I'll put in a PR to try to fix the problem. See #31 |
I'm leaving the needs discussion label, not because there hasn't been discussion but because the final version of the PR needs careful consideration. |
According to telecon and different suggestions, I've just changed CSS for the tables here. If you accept this layout for small devices, I will implement these changes in other specs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty readable across a variety of devices.
I have prepared other PRs (some of them I improved more if needed). So I'm waiting for 'yes'. |
I thought the changes were going to be conditioned on the type of device. Why is this not happening? It appears that the width of tables are being restricted to 80% of something. This seems wrong to me. Why not allow tables to be as wide as that thing? This document is not a very good one for testing out the changes as there are only a few tables with more than one column and none of them are very wide. |
Looks far better to me, overall. I see breaks are not happening mid-word. This does mean that some side-scrolling will be necessary in some smaller-windowed environments. Echidna errors appear to have been resolved. @pfps — I'm guessing that "It appears that the width of tables are being restricted to 80% of something" because you see |
As far as I can tell, the 80vw is limiting the width of tables in some places and, again as far as I can tell, the limit is active. For example, in the PR preview for some window widths the RDFS Semantic Conditions table is narrower than the text width and some lines overflow when at the same width they do not overflow without the PR. So, again, why limit table width? |
Because 80vw is the default width in respec for the content. Note that 10vw are left and right margin. |
This does not match with what I see. With the PR the width of the RDFS Semantic Conditions table is narrower than without the PR. So if there is a change, how can 80vw be the default? In any case, if 80vw is the default then there should be no reason to include it (as it would currently be doing nothing) and a good reason to not include it (as it would override future changes in RESPEC). |
I wonder if it wouldn't be worth creating a (or a few) separate PR for testing, perhaps made against one of our documents with more tables, especially tables that are problematic before whatever CSS tweak we find to be best? |
@pfps — If you could provide links (including fragment ID) to the table(s) you're seeing as problematic, that will help me (and, I imagine, others) to dig into possible solutions. |
As stated above, it's the RDFS semantic conditions table. As far as I can tell there is no direct link to it. I've always either scrolled through the document or searched for the caption. |
@pfps — Do you mean the table immediately above this anchor? (which should indeed have its own ID! as I've added in PR#33), that has only one column? Or the table following this anchor, now also at this direct link from PR#33 that has 3 columns of odd widths (left and right columns have odd wraps, and center could lose a bit of width to eliminate those wraps)...? |
It's hard to see the difference between those screenshots, with them stacked vertically. This may be helpful to others:
I agree with @pfps that this makes the tabular data harder to read, even at the substantially blown-up size seen in these screenshots, vs the font sizes seen in the actual document. |
@TallTed OK, I propose to accept the PR (which is about CSS) and open a new one to refractor HTML. Yellow, pink, etc areas are the tables, and they shouldn't be (in HTML a table element is for tabular data and it is not for tabular data). So I propose to change HTML tables there to block elements (that should look like the normal text including font size). |
@domel — I agree that the pictured tables shouldn't be tables. Replacing these with block elements seems reasonable, presuming the background and text color effects are preserved. That said, I think that the font size changes observed by @pfps are likely CSS-related, so I think @pfps should look at the actually tabular data in the other tables, and see whether they do the same font-size-shrinking with the same action — as if they do, this issue #30 should remain open and the CSS driving that should be addressed. |
All 10 PRs follow the same font size rules as this PR. Of course, if you want, I can change it, but it will be an exception. The other specs (that you have accepted) do just that. So what is the final decision? |
@domel — Have you observed the font size changes reported by @pfps when changing windows sizes? They're a problem, wherever they may occur. I am surprised that they only occur (or have only been reported) in one document and by one user — so maybe we should check that by getting browser and OS details, which may be having an impact. Maybe we should have other people check with that browser on their own OS, and have that person check other browser(s) on their OS. Please understand that I'm not deciding nor dictating anything — only confirming that an issue reported by another viewer appears to be an actual issue, which should be resolved by some means, and CSS seems the right place for that fix. Elsewhere, I've reported my observations that the CSS changes there appear to have had the desired effect, but other viewers may report differently, and their observations should not be treated as any less important than mine. |
@TallTed I can't agree that this is a problem, on the contrary, thanks to this, the text has a chance to fit on small screens. You can go into the style editor in your browser (probably by pressing F12) and check that increasing the font size destroys responsiveness (in all spec with tables). In short, I can't do it the way you want it. RWD is not about making the website look identical on a small smartphone and a huge monitor on a desktop, it should look so that it is legible. |
Again, I did not say "increase the font size". I (and @pfps) said "When the viewport/window reaches some width smaller than full-screen, the font size of the content of these tables gets smaller, while the font size of the surrounding body remains as it was." That remains a problem. |
No, it's normal behavior if you have media queries. It's the main feature of media queries that when it grows to its size, it changes styles. In this case, among other things, font size. You will find it on all websites in the world. |
This is trivially provable as inaccurate. At best, it is accurate only for those web pages which use "media queries" which is far from "all websites in the world." Further, your response to @pfps said that in his screenshots, the font sizes were identical throughout, and you couldn't understand why he saw otherwise. Now you're saying, the smaller font size in the tables is the desired and intended effect. @pfps and I are both saying, that is an undesirable and problematic effect! |
From the beginning I say that the font is smaller by I reduced it myself. Thanks to this, the spec is finally responsive and I can view it on my phone right now. |
However, I do not understand this fuss about fonts a bit, since you agree with me that the yellow areas should be rewritten in HTML (as a block elements) and then they will meet the assumptions of RWD and will have a large font. Everything you want. |
I guess we should |
From a recent version of the PR:
So, although I am not a CSS expert by any means, it appears to me that the font size of all tables was forcibly changed to 12pc from whatever it would otherwise be when the window width was under a certain value. In all cases that I managed to test, this resulted in smaller fonts in tables. Why did no-one else notice? Probably because they didn't test this PR where it actually changed the display of the document. |
Not true. Here is the recent version
|
@pchampin will talk to the accessibility team at W3C to get some input on the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the dynamic font size change to be removed. I may request other changes if that resizing is determined to be appropriate by a11y
/accessibility folks.
Trying to move forward with this...
In the meantime, I think the problem of this PR is that it tweaks too many parameters at the same time (wrap, font-size, width). My strategy would be to change one parameter at a time, and evaluate at each step the pros and cons. As an example:
|
@pchampin I don't understand what is wrong with one-column tables. I also don't know what would replace one-column tables. |
I agree that the difference can be blurry and somewhat subjective, but a pedantic reading of the HTML spec excludes the use-cases above:
(emphasis is mine) |
I'm a bit puzzled how tables can represent data with, say, 1000 dimensions. So I don't give much weight to that statement. As far as I can tell, tables are good for representing sequences in a way that emphasizes the commonality of the elements of the sequence. However, if there is a way of showing a sequence that puts the elements in equal-width boxes with no extra space between them, I'm not adverse to changing the HTML (even though it will be a pain for essentially no gain). There are several tables that have multiple rows and multiple columns so it seems that there is content that is ideally suited be tables. |
Yes, it is very easy to replace with any block element. |
Can someone provide me an example of how a block can be used to substitute for one-element and one-dimensional tables? |
I discussed this issue with @bert-github, and my take-home messages are the following:
|
The remaining problem is that changes were made to the other documents, contrary to what was supposed to happen. These changes should be undone. |
Preview | Diff