search

ably / docs

Ably Realtime API documentation
https://ably.com/docs
Apache License 2.0
20 stars 41 forks source link

[WEB-3923] - Fix Language dropdown positioning for single item #2243

Closed aralovelace closed 3 weeks ago

aralovelace commented 1 month ago

Description

WEB-3923

Review

Refactor the LanguageDropdownSelector and simplify the option display Label and should use flex even if it has no label

Also update the test after the Javascript version in the textile has been updated

Instructions on how to review the PR.

m-hulbert commented 1 month ago

That is such a nice and simple fix, however I wonder about the UX:

Monosnap Connections 2024-08-16 16-49-29

@m-hulbert wdyt?

Yeah, if we can have the language name as well for consistency I think that would be good, please @aralovelace 🙂

aralovelace commented 4 weeks ago

@m-hulbert @kennethkalmer The text "Javascript" is not displaying at the single drop-down because there's only one option in the drop-down box. Based on how the language dropdown component is being designed, I suggest, that if there is only 1 language in the dropdown we will disable it. There is no point in clicking the dropdown if it's only 1 and it is the default

jamiehenson commented 4 weeks ago

Looks okay to me, but will wait to stamp this until the most recent comment above is resolved

m-hulbert commented 4 weeks ago

@m-hulbert @kennethkalmer The text "Javascript" is not displaying at the single drop-down because there's only one option in the drop-down box. Based on how the language dropdown component is being designed, I suggest, that if there is only 1 language in the dropdown we will disable it. There is no point in clicking the dropdown if it's only 1 and it is the default

I have no objections to that @aralovelace, though I think if we disable the dropdown (as I can see you've done), then we should probably hide the chevron too.

aralovelace commented 4 weeks ago

@m-hulbert @kennethkalmer The text "Javascript" is not displaying at the single drop-down because there's only one option in the drop-down box. Based on how the language dropdown component is being designed, I suggest, that if there is only 1 language in the dropdown we will disable it. There is no point in clicking the dropdown if it's only 1 and it is the default

I have no objections to that @aralovelace, though I think if we disable the dropdown (as I can see you've done), then we should probably hide the chevron too.

@m-hulbert thanks for checking. The thing is it's part of the dropdown so we can't remove it. What we can do is just display it in a container with the same styling as the dropdown. if more than one language then use the dropdown, what do you think? or this is much easier to change the indicator color to grey if it's disabled like this:

image

aralovelace commented 3 weeks ago

thanks @m-hulbert

Can I get a final code review from either @kennethkalmer or @jamiehenson please thanks