Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: unselect selector #261

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

nervo
Copy link
Contributor

@nervo nervo commented May 29, 2024

Introduce a new style for unselected select options.

You may ask why ?

Well, before 😞 :

Capture d’écran 2024-05-29 aΜ€ 19 53 55

And, after 😍 :

Capture d’écran 2024-05-29 aΜ€ 19 53 23

With only

	t.Focused.SelectSelector = lipgloss.NewStyle().SetString("● ")
	t.Focused.UnselectSelector = lipgloss.NewStyle().SetString("β—‹ ")

⚠️ i'm not totally satisfied by the name UnselectSelector... Any opinions here ?

@nervo nervo requested a review from maaslalani as a code owner May 29, 2024 18:00
@maaslalani
Copy link
Member

I would perhaps use Cursor terminology here: SelectedCursor & UnselectedCursor?

@maaslalani
Copy link
Member

Not 100% if we want this as well since this makes it look like you can select multiple options.

@nervo
Copy link
Contributor Author

nervo commented May 29, 2024

@maaslalani it's just a use case as an example :) None of these pseudo radio buttons are in this pull request, in facts, this is fully backward compatible, as the base "UnselectSelector" is " ", just like it was hard-coded.

@joaogmguimaraes
Copy link

This modification results in a breaking change.
Previously, strings.Repeat supported customizations of SelectSelector with sizes larger than 2 chars.

@nervo
Copy link
Contributor Author

nervo commented Jun 4, 2024

@joaogmguimaraes i see what you mean. If you set SelectSelector to a size different than 2 chars, and don't touch UnselectSelector, then you break your view:

  foo
--> bar
  baz

In general, you will always got the same issue when SelectSelector and UnselectSelector are not the same chars size :(

We can address this by programmatically adding white spaces at the end of the shorter one. What do you think ? Is it worth it ?

@joaogmguimaraes
Copy link

I think so.

When UnselectSelector is smaller than SelectSelector, we should always make them equal to maintain the existing functionality. Now, if SelectSelector is smaller, I don't see the need to adjust the size (although I think it is recommended to maintain consistency).

@joaogmguimaraes
Copy link

In this library: survey

Users can add icons of different sizes, but in our case, since there is already the functionality to keep the UnselectSelector the same size as the SelectSelector, we should at least maintain this functionality working.

@nervo
Copy link
Contributor Author

nervo commented Jun 5, 2024

@joaogmguimaraes i've just pushed a naive implementation of cursors width harmonization

@maaslalani about your renaming proposal
SelectSelector -> SelectedCursor
UnselectSelector -> UnselectedCursor
Is that ok for you ? What about current SelectSelector, should we deprecate it ?

@maaslalani
Copy link
Member

I think we may actually want to name this UnselectedPrefix? What do you think @nervo?

@nervo
Copy link
Contributor Author

nervo commented Jun 21, 2024

@maaslalani i guess you mean the new UnselectSelector should be named UnselectedPrefix, am i right ?

In this case, what about the current SelectSelector, should we deprecate it in favor of something like SelectedPrefix ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants