introduce optional separator #8
Conversation
| (e.g. `| ^B |`) | ||
|
|
||
| ```tmux.conf | ||
| set -g @prefix_highlight_separator '|' |
There was a problem hiding this comment.
What do you think about defining a prefix and suffix rather than a separator and two flags to control their position?
There was a problem hiding this comment.
@erickpintor Sure, that sounds reasonable. It's also a bit more flexible. If I find some time, I'll take a pass at that tomorrow.
There was a problem hiding this comment.
It is looking very good. I just have a few more considerations regarding extra spaces.
They tend to increase and decrease the size of the status bar if we're not careful with them. It's not a big deal, it's mostly cosmetic but, some people can get annoyed with it.
| local copy_with_optional_affixes="Copy" | ||
|
|
||
| if [[ "off" != "$output_prefix" ]]; then | ||
| local prefix_with_optional_affixes="$output_prefix $prefix_with_optional_affixes" |
There was a problem hiding this comment.
It is looking very good! My only observation is to not add any extra space here or on the lines below.
Some people may use an emoji or a special character as a separator and they might want to control the spacing by themselves.
| if [[ "on" = "$show_copy_mode" ]]; then | ||
| local -r fallback="${copy_highlight}#{?pane_in_mode,Copy,}" | ||
| local -r fallback="${copy_highlight}#{?pane_in_mode, $copy_with_optional_affixes, }" |
There was a problem hiding this comment.
I remember when we added the Copy mode we didn't add a space before the word because copy has the exact same size as the prefix. It avoid increasing the size of the status bar when we enter copy mode.
There was a problem hiding this comment.
Makes sense. I'll remove the extra space.
| else | ||
| local -r fallback="" | ||
| fi | ||
|
|
||
| local -r highlight_on_prefix="${prefix_highlight}#{?client_prefix, $prefix ,$fallback}#[default]" | ||
| local -r highlight_on_prefix="${prefix_highlight}#{?client_prefix, $prefix_with_optional_affixes, $fallback}#[default]" |
There was a problem hiding this comment.
I'm not sure. But I think the space before $fallback will be considered too. We may want to avoid it.
There was a problem hiding this comment.
It looks like the spacing used on the old line 46 (?client_prefix, $prefix ,$fallback) contains an additional space. I can either revert to using that and we can take up the spacing problem (if it is one ...) in another issue or address it here by removing all of the spaces. In the latter scenario, the string would be: "${prefix_highlight}#{?client_prefix,$prefix_with_optional_affixes,$fallback}#[default]"
There was a problem hiding this comment.
I think we can remove all spaces and since we now have prefix and suffix properties, people can add them back if they feel like it.
There was a problem hiding this comment.
Perhaps we make the default prefix and suffix as a single space? Just to keep backwards compatibility? It's up to you. :)
There was a problem hiding this comment.
Perhaps we make the default prefix and suffix as a single space? Just to keep backwards compatibility?
That's a great idea. It also removes the need for some of the conditionals and variable re-assignment I was using.
Addresses #7.