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

Add theming to toolbar options #645

Merged
merged 6 commits into from
Jul 24, 2024

Conversation

sudhanshutech
Copy link
Member

@sudhanshutech sudhanshutech commented Jun 8, 2024

Notes for Reviewers

This PR adds theme to toolbar components which were missing.
image
image

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Sudhanshu Dasgupta <[email protected]>
@sudhanshutech sudhanshutech added the pr/do-not-merge PRs not ready to be merged label Jun 8, 2024
Signed-off-by: Sudhanshu Dasgupta <[email protected]>
@sudhanshutech sudhanshutech removed the pr/do-not-merge PRs not ready to be merged label Jun 9, 2024
Signed-off-by: Sudhanshu Dasgupta <[email protected]>
@Rexford74
Copy link
Contributor

This looks largely good. There are two things I'd like to point out though.
First, what text token are you using here.
Second, the unchecked state for the checkbox doesn't have a green outline on it. I am aware that this is the initial version but after the last meeting that I attended on Friday, I made changes to the checkbox in Figma to include the green outline. I wonder if there has been a reconsideration on this stance...

@Deepak0320Singhal
Copy link

Hi @sudhanshutech, thank you for raising this fix, looks we missed covering this on any sites call.
Let's discuss this on Websites's call on today. Adding this as an agenda item into the meeting minutes.

@leecalcote
Copy link
Member

@sudhanshutech thoughts on @Rexford74's feedback?

@sudhanshutech
Copy link
Member Author

sudhanshutech commented Jul 12, 2024

@leecalcote i tried the requested design of checkbox , here is the checbox screenshot.
image

What i discovered from this is we should stick with our existing checkbox which is single border in a unchecked state.
Reason: is the checkbox appears quite small with the double outline, making it challenging for users to easily distinguish the clickable area specially in mobile screen it will hard for user to distinguish these two borders and will look might be confusing and cluttered.
The existing with single outline gives a breathing space. These were my findings and However, if needed, I can implement the style.

Let me know your thoughts // @Rexford74

@leecalcote
Copy link
Member

@leecalcote i tried the requested design of checkbox , here is the checbox screenshot. image

What i discovered from this is we should stick with our existing checkbox which is single border in a unchecked state. Reason: is the checkbox appears quite small with the double outline, making it challenging for users to easily distinguish the clickable area specially in mobile screen it will hard for user to distinguish these two borders and will look might be confusing and cluttered. The existing with single outline gives a breathing space. These were my findings and However, if needed, I can implement the style.

Let me know your thoughts // @Rexford74

@sudhanshutech demo this issue today, please.

@sudhanshutech
Copy link
Member Author

Ah sorry i missed, I will do surely next time.

…o toolbar/options

Signed-off-by: Sudhanshu Dasgupta <[email protected]>
…o toolbar/options

Signed-off-by: Sudhanshu Dasgupta <[email protected]>
Signed-off-by: Sudhanshu Dasgupta <[email protected]>
@sudhanshutech sudhanshutech merged commit a86627e into layer5io:master Jul 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants