Indent 'quoted' text based on level of quoting. #380
Conversation
|
@sumanthvrao This is a good WIP in that it shows indents of any kind. I'm wondering if we should try a different character for the indent though. I gave it a manual try and it seems to work for multiple levels quite well, though not for multi-paragraph indents. |
|
Thanks for the quick feedback @neiljp .Yes making it work for multi-paragraph indents seems to be the next challenge here. Particularly because, it may involve tackling the problem with a different approach. For now I am trying to get this much up and working. Which character do you suggest? |
|
For character, I suppose I'm just wondering if something different to the 'side marker' would be good; there is a difference in color but a character difference would maybe be useful too. |
sumanthvrao
force-pushed
the
zt-quoted-text
branch
from
ebb3d9f
to
c6b78e3
Compare
|
Updated a slightly better version than before. It now works for multi-paragraph indents, though barely (Only the first line gets indented). I changed the logic from the previous iteration and it now takes lesser code to do the same thing. I think getting to the level of web-app with regard to this aspect is challenging. @neiljp Agreed, we can go for a different padding character. |
sumanthvrao
force-pushed
the
zt-quoted-text
branch
from
c6b78e3
to
6fce455
Compare
|
@neiljp Pushed a new iteration of this. It now works for multi-line quoting and normal (multi-depth) quoting. For text which naturally wraps (due to length restriction), this does not work so well. But that said, I don't know if this logic can handle that 'easily'? Let me know how this feels. We can then decide whether to stick to this or try another logic. TODO:
|
sumanthvrao
changed the title
sumanthvrao
force-pushed
the
zt-quoted-text
branch
from
6fce455
to
ca4969e
Compare
|
Just tried this. This also doesn't seem to be working so well for messages with links in them. |
|
Thanks for the report @shreyamalviya. This seems like the normal wrapping of text issue I was talking about. Will get back to you after some more digging to see what the actual problem is. Btw, I see a number of shades (of color) here; especially the quoted text. What theme are you using? |
|
I'm using the default theme. |
|
If we can have this tested and working for things like links, I think this should be good as v1 version of this - I think it's definitely an improvement over the current state of master. There is an issue with extra top lines, but apparently that's in master too, so could be a separate fix. |
We display the quoted text with padding/indentation, so that it is easier to distinguish between replied text and message. The level of indentation depends on the level of quoting.
The incoming message (soup) for blockquotes contains '\n' characters at various places. Some of those which occur later on are useful (act as seperator) while the first few used to cause a large gap at the starting of the message. Now, we only allow the later occuring newline characters to conserve space;
sumanthvrao
force-pushed
the
zt-quoted-text
branch
from
ca4969e
to
fd2154d
Compare
|
A small commit which handles removal of the extra space on the top has been added. This is more of a repair for the bug, rather than avoiding it to begin with. I could not come up with any other way to avoid the gap at the beginning; Especially, provided that the stuff we can do with This sort-off works for links now; Though I am not sure to what extent we can perfect this. Tests are yet to be added; I did a bunch of manual testing and it seems like a general improvement overall. |
sumanthvrao
changed the title
|
@neiljp I gave this another trial run, I am okay with this being as is, if it doesn't look too bad or causes any regressions, what do you think? |
|
@amanagr Could you take a look at this? @sumanthvrao Did you resolve the issue with links? Did we get a test case for this? (@shreyamalviya ?) |
|
This is looks good for now as a first step. I think next step would be to modify MessageBox to be able to include other widgets like ListBox, Checkbox(to include poll) and any other widget basically inside it. |
|
@neiljp I did not find a test case for this. The color issue which is there in the photo @shreyamalviya posted, seems to exist in the current Master as well, so I am guessing it is something separate from this. @amanagr Good to know. I am okay if we want to test-deploy it to, gain some more feedback. |
|
Heads up @sumanthvrao, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
@sumanthvrao Thanks for working on this, it's a definite improvement 👍 I've merged what we have to get wider exposure, adjusting the quote character, an apparent off-by-one error in the line-removal and some commit text. If you feel like developing some tests I think that would help with narrowing down cases of unexpected behavior compared to the webapp, and ensuring we don't regress. We don't have a tracking bug for improving quoting, but that seems reasonable to file given the discussion around using |
This is a very early implementation of displaying quoted messages in a better manner.