Greater 50 chars commit highlighting #197
Conversation
There was a problem hiding this comment.
Hi @ciaranmooney thanks for getting at this. Please be aware this component is used as a general text input and we likely wanna be able to make the limit highlighting optional / configurable.
Also this component already had lots of bugs with cutting in between utf8 boundaries so I would ask you to add some unittests to show the changes do not suffer something similar.
| fn apply_highlighting(&self) -> Vec<Text> { | ||
| let style = self.theme.text(true, false); | ||
| let mut txt = Vec::new(); | ||
| let char_limit_warning = 50; |
There was a problem hiding this comment.
for constants lets put them up on top outside the type
| let char_limit_warning = 50; | ||
| if self.msg.len() > char_limit_warning { | ||
| txt.push(Text::styled( | ||
| &self.msg[..char_limit_warning], |
There was a problem hiding this comment.
this will likely fail if the limit cuts in the middle of a utf8 boundary.
There was a problem hiding this comment.
Didn't even consider utf8 - embarrassing considering I have one (á) in my name.
There was a problem hiding this comment.
no worries, happens to me all the time 🙈
| )); | ||
| txt.push(Text::styled( | ||
| &self.msg[char_limit_warning..], | ||
| style.fg(Red), |
There was a problem hiding this comment.
colors should be looked up using the theme so people can customise it. in the style we have text_danger already which likely is what u are looking for
| @@ -166,7 +186,8 @@ impl DrawableComponent for TextInputComponent { | |||
| self.theme.text(false, false), | |||
| )] | |||
| } else { | |||
| self.get_draw_text() | |||
| self.apply_cursor(); | |||
There was a problem hiding this comment.
how come we don't need to use the result of apply_cursor?
| @@ -104,13 +105,13 @@ impl TextInputComponent { | |||
| self.title = t; | |||
| } | |||
|
|
|||
| fn get_draw_text(&self) -> Vec<Text> { | |||
There was a problem hiding this comment.
I think get_draw_text is more appropriate for what we want the method to do, if we wanna split the logic it does because it would be too much for one method lets subdivide it. other components already use a similar notation of moving this logic into a sort of get_text method.
|
Hi, Thanks for taking the time to review this PR. I must admit this was the first bit of Rust I have written. I was overcome with my own inflated sense of achievement that I decided to offer it up. TODOs
|
lets just worry about the use cases we have: commit message (limit 50), stash name (no limit), upcoming tag title (no limit), so I guess a I am just saying cause those words could mean many more things otherwise :) |
|
@ciaranmooney are you still planning on pursuing this? |
|
Hi, Yes. I have recently come back to it. I'm happy to work on it privately on a my own repo for the time being if you wish to close this PR down. |
|
After making 3 or 4 attempts at implementing highlighting properly I'm going to have to admit defeat, for now. It seems that adding styling based on two separate conditions (cursor and character limit) is not quite as simple as I'd like it to be. I can't seem to find a way that does not end in a untidy mess of if-statements and magnitude checks (ie limit < length, limit < cursor_position etc). I believe that this component may need some refactoring. Unless there is another area of the application which handles a similar scenario? I have extended the unit tests to cover existing behaviour so I will endeavour to get them merged in a separate PR. |
|
closed in favour of #264 |
This PR adds a red highlight to characters that exceed the limit of 50 in a commit message (see second point of #46).
In order to accomplish this I have renamed the
get_draw_textfunction toapply_cursoras this seemed more apt and allowed me to add in theapply_highlightingfunction.Along the way I corrected some minor typographical errors in comments.