Fixes length issues, capital alignment issues, author name overlap issue #5
Conversation
| // 75 characters above spoils the alignment | ||
| if (titles[i].innerHTML.length > 75) { | ||
| titles[i].innerHTML = titles[i].innerHTML.substring(0, 73) + "..."; | ||
| } |
There was a problem hiding this comment.
Isn't this more elegantly achieved using text-overflow: ellipsis; (and possibly some other CSS properties) ?
There was a problem hiding this comment.
text-overflow: ellipsis is a good property but I could not get it to work even with other properties. However I found this in the MDN docs
The text-overflow property only affects content that is overflowing a block container element in its inline progression direction (not text overflowing at the bottom of a box, for example).
There was a problem hiding this comment.
Also just a question - What is the problem with JavaScript? (A generalized question for both the comments)
There was a problem hiding this comment.
No problem with JS, I think it's more about just using the right tool for the job.
I'd like to have a go at seeing if text-overflowcould work, hope that's ok. Otherwise this should be an okay way to deal with it. I appreciate the help 👍
There was a problem hiding this comment.
Hey, I just had a go at trying to fix this and yeah, it's very difficult to get it perfect using just CSS. I got close but not quite. The issue is dealing not just with long titles but specifically long words.
Anyway, your solution works well, however your chosen values didn't work for me. I had to reduce the character length to 40 in the JS. I experimented with various different titles with long words, many short words, etc... Are you able to confirm?
There was a problem hiding this comment.
40 instead fits perfectly for me and my chosen values goes out of bounds
Browser - Firefox v100
Size - 1920x968 px
bookshelf.js
Outdated
| //If all caps capitalize the first letter of each word as all capitals also spoil the alignment | ||
| if (titles[i].innerHTML.toUpperCase() === titles[i].innerHTML) { | ||
| titles[i].innerHTML = titles[i].innerHTML.replace(/\w\S*/g, function (txt) { | ||
| return txt.charAt(0).toUpperCase() + txt.substr(1).toLowerCase(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can text-transform: capitalize; not be used instead?
There was a problem hiding this comment.
Here I am using this to only capitalize the first letter of the already capitalized string
For example - HELLO WORLD gets converted to Hello World
There was a problem hiding this comment.
Right, I don't think you need all that JS for that though.
text-transform: capitalize will take lower case text and turn it into "title case" (hello world -> Hello World) so to make it work with all caps input you can just use toLowerCase()
I'm just wondering if this is really necessary though; the user defines the input after all. Need to think about it.
There was a problem hiding this comment.
Actually yes this makes sense, I totally skipped the part where is the user is the one who enters the data. Should I put in a commit removing this piece of code?
There was a problem hiding this comment.
Yes, I think ultimately this isn't needed. However, adding:
text-transform: capitalize;
text-align: left;to .spine-title is a good addition I think.
There was a problem hiding this comment.
For some reason text-align: left; makes it look bad. The center alignment should stay I feel.
2 participants
From trial and error characters over 75 often caused alignment issues. To fix this a truncation was introduced at the cost of the title.
All caps titles also caused alignment issues. To fix this only the first letter of the word was capitalized.
Often with big titles it often used to overlap with the author name. To fix this a small padding to remove overlap has been introduced.