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

termlist table: added min-width 4em for Japanese columns; copied style from 2nd ed. #99

Merged
merged 6 commits into from
Oct 11, 2019

Conversation

himorin
Copy link
Contributor

@himorin himorin commented Jul 4, 2019

for issue #6, replace PR #59


Preview | Diff

@himorin himorin requested review from r12a and kidayasuo July 4, 2019 05:28
@r12a
Copy link
Contributor

r12a commented Jul 5, 2019

hi @himorin. Thanks for this. Here are my suggestions.

  1. if we are to use class names, i'd prefer to use semantic labels rather than presentation-related. However, i think it's easiest not to add class names at all. Just use .termlist th. It applies the style to all the headings, but that's not an issue. (Notice also that my selector just uses .termlist, rather than table.termlist. That's redundant information and sometimes can be over-specific.
  2. i'd like to also add white-space: nowrap to the .termlist th definition. This makes it look cleaner (and probably also removes the need for the 4em minimum, but that doesn't matter.)
  3. Personal preferences: i would make the padding slightly bigger and only apply to the side, ie. padding: 0 0.5em;. I also prefer to keep the framework of the table from intruding on the reader, so i'd set the border to border: solid 1px #ccc, but that's up to you.
  4. (Could be a separate issue.) If we're going to apply cell borders to this table (and i think it's a very good idea), then we should also apply them to the tables in Appendix A.

hth

@r12a
Copy link
Contributor

r12a commented Jul 5, 2019

I also just realised that the br elements we have separating the en and ja text in the headings lead to a gap at the top of the header cell when only japanese is displayed. The way to get around this, i think, is to remove the br elements, and add a css rule saying:

.termlist th span {
    display: inline-block;
    }

(We could do the same for .charclass to achieve the same for the tables in Appendix A, although i noticed that switching languages doesn't affect those headings - not sure why.)

@himorin
Copy link
Contributor Author

himorin commented Jul 8, 2019

  1. done
  2. we are somehow afraid of making the last column (description) narrower, since there are sentences but not just word and narrow display could make them difficult to read. there is several long words in names especially in pronunciation (よみ), and these could take wide space when we specify nowrap to all.
  3. changed.
  4. yes. let us have another issue. (I think we had borders in 2nd version,,,?)

@himorin
Copy link
Contributor Author

himorin commented Jul 8, 2019

I also just realised that the br elements we have separating the en and ja text in the headings lead to a gap at the top of the header cell when only japanese is displayed. The way to get around this, i think, is to remove the br elements, and add a css rule saying:

.termlist th span {
    display: inline-block;
    }

modified.

(We could do the same for .charclass to achieve the same for the tables in Appendix A, although i noticed that switching languages doesn't affect those headings - not sure why.)

I think there is no br in charclass.

index.html Show resolved Hide resolved
@r12a
Copy link
Contributor

r12a commented Jul 8, 2019

we are somehow afraid of making the last column (description) narrower, since there are sentences but not just word and narrow display could make them difficult to read. there is several long words in names especially in pronunciation (よみ), and these could take wide space when we specify nowrap to all.

You're only making the th nowrap. So the minimum width is set by the width of the heading cell, but the maximum width of the cells below is unaffected.

@himorin
Copy link
Contributor Author

himorin commented Jul 8, 2019

we are somehow afraid of making the last column (description) narrower, since there are sentences but not just word and narrow display could make them difficult to read. there is several long words in names especially in pronunciation (よみ), and these could take wide space when we specify nowrap to all.

You're only making the th nowrap. So the minimum width is set by the width of the heading cell, but the maximum width of the cells below is unaffected.

aaah, yes! updated as suggested.

@himorin
Copy link
Contributor Author

himorin commented Jul 8, 2019

ah, no. it seems we should set nowrap to span but not th itself.
let me check and get back on this later... (just a memo for myself)

@r12a
Copy link
Contributor

r12a commented Jul 8, 2019

The text content of the column headings is inline-block, so you don't need to assign to the spans.

(Btw, fwiw, i didn't suggest the not:hidden thing because this table is dual language in nature (only the descriptions get affected by language change), so i figured it's ok to have the column headers always visible in english and japanese. However, should be ok like you did it, too.)

@himorin
Copy link
Contributor Author

himorin commented Jul 8, 2019

for :not(.hidden), I was just afraid of 'the last match is taken' nature, and added not to override any pre-defined ones. so, if it is fine to be left, let me keep this as is...
for column headings, I was actually mixed several items before I've committed my changes. apologies. I wondered if I just removed br from inline content (so, en/ja are continuously displayed on 'all' display), but it should not be. And I agree that no change is required on this.

@himorin
Copy link
Contributor Author

himorin commented Jul 29, 2019

let me ask review (r?) on this with the current ecee15c .

@himorin
Copy link
Contributor Author

himorin commented Sep 10, 2019

is this PR ok to merge now?

@kidayasuo
Copy link
Contributor

Reviewed the latest branch. The "用語" column is too narrow and fits only two characters when viewed in Japanese language on MacBook 12-inch. This needs to be fixed. question: is there a way to turn off the index on the left side? With smaller screen the table is hard to read.

@r12a
Copy link
Contributor

r12a commented Oct 9, 2019

@kidayasuo I'm wondering how you reviewed it, because doing so is problematic in this case. The styling doesn't get applied properly when using the diff link above... I think the best way to review is to copy the source of the two files to a local directory and open from there.

@kidayasuo
Copy link
Contributor

@r12a I reviewed the fix by downloading the top of the branch to the local machine and opened the index.html from there. It happens only when you choose "日本語".

@kidayasuo
Copy link
Contributor

kidayasuo commented Oct 9, 2019

ah, the top of tree of course does not have the fix. How do I download the two files?
unrelated question: the default should be "English" rather than "All". Has it been fixed and still not merged? or has it not been fixed (in which case I will open an issue) ?

@xfq
Copy link
Member

xfq commented Oct 10, 2019

@kidayasuo It's only fixed in @himorin's issue-6b branch. You can:

Or:

@xfq
Copy link
Member

xfq commented Oct 10, 2019

@kidayasuo re "turn off the index on the left side", I think the Table of Contents only appears as a sidebar on screens with a wide viewport. In the case it appears, you can click/touch the leftwards arrow on the bottom left of the page to collapse the sidebar.

local.css Outdated Show resolved Hide resolved
@xfq
Copy link
Member

xfq commented Oct 10, 2019

About the the default language, I don't think it has been changed. We probably need a new issue for that.

@kidayasuo
Copy link
Contributor

@xfq thank you! and sorry for the confusion. I downloaded the zip and confirmed that the issue I described does not exist, i.e. the "用語" column is wide enough. Also confirmed that the original issue #6 is inactive. In both Japanese and English primary.

Copy link
Contributor

@kidayasuo kidayasuo left a comment

Choose a reason for hiding this comment

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

Approving the fix.

@himorin
Copy link
Contributor Author

himorin commented Oct 11, 2019

seems all set. merging this.

@himorin himorin merged commit e9573bf into w3c:gh-pages Oct 11, 2019
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

4 participants