search

DistributedProofreaders / dproofreaders

Distributed Proofreaders is a web application intended to ease the process of converting public domain books into e-texts.
https://www.pgdp.net
GNU General Public License v2.0
46 stars 28 forks source link

change character picker text-align to right #1223

Closed 70ray closed 3 weeks ago

70ray commented 1 month ago

This enables diacritical marks for polytonic greek to appear correctly in the character picker.

Sandbox at: https://www.pgdp.org/~rp31/c.branch/picker_indent

cpeel commented 1 month ago

I don't think this is a great solution given what it does to the other 99.9% of the characters:

Screen Shot 2024-05-29 at 7 42 18 AM
70ray commented 1 month ago

I don't think this is a great solution given what it does to the other 99.9% of the characters:

I agree, but it is better than doing nothing. It seems slightly less bad than giving a text indent for some reason. The only other fixes that occur to me would be to make all the boxes bigger, which we don't really want, or to put in a text indent or alignment change for selected characters which would be possible but a bit messy. Or alternatively re-work the dp sans mono font so that the the diacritical marks fit in the 'box' and use that font for the picker. This would fix this and the other issues as well. Or the other option which I mentioned in the slack discussion to change to dejavu sans (not mono) which keeps every thing centred but loses the distinction between some characters. Perhaps this would be acceptable for the character picker. Although the O and Omicron are much wider and the Omicron with diacriticals doesn't quite fit in the box.

ellinora commented 1 month ago

It did seem a bit off balance upon first glance. I can still easily find the character I'm looking for though.

windymilla commented 1 month ago

I don't really like the off-center layout for all the other characters either, though I agree that the only alternatives seem to be aligning different characters differently, fixing the troublesome characters in the DP Sans Mono font, or making the boxes a bit wider.

srjfoo commented 1 month ago

I don't think this is a great solution given what it does to the other 99.9% of the characters:

I agree, but it is better than doing nothing. It seems slightly less bad than giving a text indent for some reason. The only other fixes that occur to me would be to make all the boxes bigger, which we don't really want, or to put in a text indent or alignment change for selected characters which would be possible but a bit messy. Or alternatively re-work the dp sans mono font so that the the diacritical marks fit in the 'box' and use that font for the picker. This would fix this and the other issues as well. Or the other option which I mentioned in the slack discussion to change to dejavu sans (not mono) which keeps every thing centred but loses the distinction between some characters. Perhaps this would be acceptable for the character picker. Although the O and Omicron are much wider and the Omicron with diacriticals doesn't quite fit in the box.

For what it's worth, I agree with @cpeel for the pickers. It may be strictly an aesthetic response, but I don't agree that it's better than doing nothing. If you look at the picker set as seen in the screenshot, the diacriticals are clearly visible.

image

And, yes, that's aesthetically poor, but I think better than affecting the balance of all the other character pickers. Given that the dicriticals are actually visible, and not being clipped, do we really need to do anything at this point?

However, for the MRU box, if there's a double-diacritical before, it does actually get clipped. Would it make sense to special-case the MRU box and make it slightly larger so that the double-diacritical before does actually show? That way, you're only affecting the size of one thing, and you're not messing with the alignment (I wouldn't right-align the MRU, either).

windymilla commented 1 month ago

If the decision were taken to give characters different indents, is it possible to detect the true bounding box of a character, rather than the standard one for the mono font? If so, that would be preferable to having a list of characters that need a text-indent.

windymilla commented 1 month ago

The characters in the picker looked very slightly off-center to me even when I edited the CSS to center alignment. I think that is because there is a 1px right padding being applied at line 378 of pages_interfaces.less It's only a pixel, but if the decision is to follow @srjfoo's suggestion to let the accents spill out of the picker buttons, then you could gain a little by zeroing that right padding.

70ray commented 1 month ago

Another thing we could do is to not have separate buttons for the characters so it looks more like the DPC version image Then the diacritical marks would be visible and we could make the large character box bigger as Sharon suggested.

70ray commented 1 month ago

If the decision were taken to give characters different indents, is it possible to detect the true bounding box of a character, rather than the standard one for the mono font? If so, that would be preferable to having a list of characters that need a text-indent.

That would be good if it is possible.

70ray commented 4 weeks ago

If the decision were taken to give characters different indents, is it possible to detect the true bounding box of a character, rather than the standard one for the mono font? If so, that would be preferable to having a list of characters that need a text-indent.

It appears this is possible by drawing the characters on a canvas and using the TextMetrics: actualBoundingBoxLeft and actualBoundingBoxRight properties but these are not available in our supported browsers. Another way would be to examine the IntlChar::charName and change the alignment for greek capitals with oxia or varia.

70ray commented 4 weeks ago

This version gives a text-indent of 0.4em only for greek capitals where oxia or varia are present. It doesn't yet work on the MRU or the large character.

Sandbox updated.

srjfoo commented 4 weeks ago

This version gives a text-indent of 0.4em only for greek capitals where oxia or varia are present. It doesn't yet work on the MRU or the large character.

Sandbox updated.

I can see them just fine in the MRU buttons, even if they do spill over the edge of the button. That doesn't bother me, because it is visible. However, in the enlarged character, it's clipped. I think fixing that is more important than fixing the character picker and MRU character buttons.

windymilla commented 4 weeks ago

I like this approach too - it works well in the pickers it's implemented for. It seemed to offset all the correct characters, without offsetting any others unnecessarily.

70ray commented 4 weeks ago

MRU and big character now also indent. Sandbox updated. There is some duplication in php and javascript functions. It would be nice to avoid this by drawing the pickers in javascript using api data.

srjfoo commented 4 weeks ago

I agree that (though it's annoying to have to) it's good that we can customize on a character-level.

I played around with this a little. See the screenshot below. The two characters that are highlighted have a .3em indent instead of .4em. Compare the spacing at the right edge of those two with the spacing of the others. I think it's okay for the breathing marks to touch the left-edge; they're not being clipped. There are already other diacriticals that touch the left edge.

image

That being said, I'm fine with whatever is decided. I'm only tossing this out as a possible option.

70ray commented 3 weeks ago

I agree that (though it's annoying to have to) it's good that we can customize on a character-level.

I played around with this a little. See the screenshot below. The two characters that are highlighted have a .3em indent instead of .4em. Compare the spacing at the right edge of those two with the spacing of the others. I think it's okay for the breathing marks to touch the left-edge; they're not being clipped. There are already other diacriticals that touch the left edge.

We could compromise on 0.35em.

srjfoo commented 3 weeks ago

We could compromise on 0.35em.

If it's okay with everyone else, I sort of prefer this, but I'm okay with .4em if everyone else prefers it. Apologies for not responding earlier.

I do have a question, though. What actually got the discussion about missing diacriticals on Greek letters started was Task #2081, not the character pickers. Should that be fixed with this PR, also? (Fortunately, it should be straightforward, as the WordCheck input box isn't as cramped for space as the pickers are.)

70ray commented 3 weeks ago

We could compromise on 0.35em.

If it's okay with everyone else, I sort of prefer this, but I'm okay with .4em if everyone else prefers it. Apologies for not responding earlier.

Changed to 0.35em and added comments referring to duplicated code. Sandbox updated.

I do have a question, though. What actually got the discussion about missing diacriticals on Greek letters started was Task #2081, not the character pickers. Should that be fixed with this PR, also? (Fortunately, it should be straightforward, as the WordCheck input box isn't as cramped for space as the pickers are.)

We could perhaps make another PR for that.