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.

Sign up for GitHub

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

Jump to bottom

feat: Adding TextBox Icons Command and Visibility #997

Merged
merged 13 commits into from
Nov 21, 2023
Merged

feat: Adding TextBox Icons Command and Visibility #997

merged 13 commits into from
Nov 21, 2023

Conversation

eriklimakc
Copy link
Contributor

@eriklimakc eriklimakc commented May 25, 2023 •
edited
Loading

GitHub Issue: #996

PR Type

What kind of change does this PR introduce?

  • Feature

Description

Added new DP in ControlExtensions:

LeadingIcon
IsLeadinIconVisible
LeadingCommand

TrailingIcon
IsTrailingIconVisible
TrailingCommand

PR Checklist

Please check if your PR fulfills the following requirements:

  • Commits must be following the Conventional Commits specification
  • Tested UWP
  • Tested iOS
  • Tested Android
  • Tested WASM
  • Tested GTK
  • Tested MacOS
  • Contains No breaking changes

    If the pull request contains breaking changes, commit message must contain a detailed description of the action to take for the consumer of this library. As explained by the Conventional Commits specification

Other information

Internal Issue (If applicable):

@eriklimakc eriklimakc self-assigned this May 25, 2023
@eriklimakc eriklimakc linked an issue May 25, 2023 that may be closed by this pull request
[Material][TextBox] Add support for Leading/Trailing Icon/IconCommand #996
Open
@github-actions
Copy link

github-actions bot commented May 25, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@eriklimakc eriklimakc requested a review from kazo0 May 26, 2023 09:49
@github-actions
Copy link

github-actions bot commented Jun 2, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@eriklimakc eriklimakc requested a review from kazo0 June 2, 2023 11:42
@github-actions
Copy link

github-actions bot commented Jun 2, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

kazo0
kazo0 reviewed Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@kazo0 kazo0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to create a new page in the docs for this too :)

agneszitte
agneszitte reviewed Jun 13, 2023
View reviewed changes
Copy link
Contributor

@agneszitte agneszitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eriklimakc , @kazo0, @Xiaoy312 (cc @NVLudwig)
We have some padding inconsistency here
image

@NVLudwig if you can confirm, please

image

Xiaoy312
Xiaoy312 reviewed Jun 13, 2023
View reviewed changes
Copy link
Contributor

@Xiaoy312 Xiaoy312 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are also relevant entry in the doc, that you should add/update as well: https://github.com/unoplatform/Uno.Themes/blob/master/doc/material-controls-extensions.md
(maybe after you have go through the comment first, as it will impact the doc)

src/library/Uno.Material/Extensions/TextBoxExtensions.cs Outdated Show resolved Hide resolved
@NVLudwig
Copy link

NVLudwig commented Jun 13, 2023 •
edited by unfurl-links bot
Loading

@eriklimakc , @kazo0, @Xiaoy312 (cc @NVLudwig) We have some padding inconsistency here image

@NVLudwig if you can confirm, please

image

@eriklimakc @agneszitte
Good catches eagle eyed Agnes.

First of all, let it be known that "hell is a textbox", it looks deceptively simple but it really isin't AND it's really important to get it rock solid perfect, so thank you all for working so very hard on this one, I know how frustrating this can get.

Here are a few ansers I hop help you along, contact me if more discussion is needed:

1a) Padding should the same before leading icon and after trailing icon
1b) Padding should be the same whether or note there is a leading icon (see recording I did with Password box that ahs the same pattern)

One.padding.to.rule.all.textboxes.mov
  1. Yes you can have a clear button followed by a second trailing button
Material Design
Text fields allow users to enter text into a UI. They typically appear in forms and dialogs.

@kazo0
Copy link
Collaborator

kazo0 commented Jun 13, 2023

@carldebilly / @Robert-Louis FYI we have breaking changes here. The TextBox won't be using the ControlExtensions.Icon property anymore

@eriklimakc eriklimakc force-pushed the dev/ERLI/303-AddTextBoxTrailingIcon branch from b89d153 to fe2ad20 Compare June 19, 2023 10:36
@github-actions
Copy link

github-actions bot commented Jun 19, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@agneszitte agneszitte marked this pull request as draft August 8, 2023 16:13
@eriklimakc eriklimakc force-pushed the dev/ERLI/303-AddTextBoxTrailingIcon branch from fe2ad20 to 0d4015e Compare September 6, 2023 17:28
@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@NVLudwig
Copy link

NVLudwig commented Sep 6, 2023

@eriklimakc Icons should be the same color: OnSurfaceVariantColor
image

@github-actions
Copy link

github-actions bot commented Sep 7, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

1 similar comment
@github-actions
Copy link

github-actions bot commented Sep 7, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@eriklimakc
Copy link
Contributor Author

To adjust:

  1. Is .../..Shared/Entities/Data/ the right place fot TestCommands to be?

  2. How to prevent the page from scrolling up when clicking in Leading or Trailing icon on the ControlExtensions sample page? You can check from the Static WASM Uno App here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net/

cc @kazo0 @agneszitte @Xiaoy312

@Xiaoy312
Copy link
Contributor

Xiaoy312 commented Sep 7, 2023

@eriklimakc re: #997 (comment)

  1. No. You should not have specialized command. Use the lambda version where you can pass an action () => MagicHere().
  2. This is not supposed to happen. We have an similar issue with ToggleSwitch on wasm as well recently (this week), touching it causes the page to scroll up . Although, I can't seem to find the issue for it right now, or it was never created. Can you open an issue for this?

@eriklimakc eriklimakc marked this pull request as ready for review September 7, 2023 16:26
@github-actions
Copy link

github-actions bot commented Sep 7, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@eriklimakc eriklimakc mentioned this pull request Sep 20, 2023
Closed
10 tasks
@eriklimakc eriklimakc force-pushed the dev/ERLI/303-AddTextBoxTrailingIcon branch from 170bd0e to 7c72ee3 Compare November 2, 2023 15:21
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 2, 2023

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@github-actions GitHub Actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://salmon-rock-0cfebe70f-997.eastus2.azurestaticapps.net

@kazo0 kazo0 merged commit 0b2a7a9 into master Nov 21, 2023
11 checks passed
@kazo0 kazo0 deleted the dev/ERLI/303-AddTextBoxTrailingIcon branch November 21, 2023 22:10
kazo0 added a commit that referenced this pull request Dec 1, 2023
@kazo0
Revert "feat: Adding TextBox Icons Command and Visibility (#997)"
d21d252 
This reverts commit 0b2a7a9.
kazo0 added a commit that referenced this pull request Dec 1, 2023
@kazo0
chore: revert "feat: Adding TextBox Icons Command and Visibility (#997)"
a23c7e3 
This reverts commit 0b2a7a9.
kazo0 added a commit that referenced this pull request Dec 1, 2023
@kazo0
chore: revert "feat: Adding TextBox Icons Command and Visibility (#997)…
79e3d3b 
…" (#1296)

This reverts commit 0b2a7a9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Marc-Antoine-Soucy Marc-Antoine-Soucy Marc-Antoine-Soucy left review comments

@kazo0 kazo0 kazo0 approved these changes

@Xiaoy312 Xiaoy312 Xiaoy312 left review comments

@agneszitte agneszitte Awaiting requested review from agneszitte

Assignees

@eriklimakc eriklimakc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Material][TextBox] Add support for Leading/Trailing Icon/IconCommand
6 participants
@eriklimakc @NVLudwig @kazo0 @Xiaoy312 @agneszitte @Marc-Antoine-Soucy