Skip to content

Fix TokenizingTextBox header to behave like normal text control headers #4101

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

Conversation

marcelwgn
Copy link
Contributor

Fixes #4096

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Header rendered inside the TokenizingTextBox

What is the new behavior?

Header now renders above the TokenizingTextBox input area.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

Also took the liberty to add the properties for the header presenter that are also present for the TextBox header presenter.

@ghost
Copy link

ghost commented Jul 8, 2021

Thanks chingucoding for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Rosuavio July 8, 2021 19:56
@ghost ghost added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Jul 8, 2021
@@ -95,10 +111,7 @@
Margin="{StaticResource TokenizingTextBoxPresenterMargin}"
Footer="{TemplateBinding Footer}"
Copy link
Member

Choose a reason for hiding this comment

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

Would we want Footer be also abstracted here? Otherwise it'll be in the TextBox part again, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funnily, in my testing, setting the footer does not actually change anything. So should we move the footer out of the ItemsControl like the header too?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think while we're at it here, it makes sense. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@Rosuavio
Copy link
Contributor

Rosuavio commented Sep 2, 2021

@michael-hawker I see that this was removed from the Bugs7.1 board, but the related issue is on that board, is there are reason that this PR is not on the board?

@ghost ghost removed the needs attention 👋 label Sep 2, 2021
@michael-hawker
Copy link
Member

michael-hawker commented Sep 2, 2021

@RosarioPulella we only put PRs on the board that have no corresponding issue. If there's an issue for the PR, then the issue is what stays on the board. If we have both a PR and an issue on the board the board gets cluttered with duplicates. 🙂

That's why we also set the milestone, as the PR is still in the 7.1 milestone to match too.

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Thanks @chingucoding for fixing this! 🎉🎉🎉

@michael-hawker michael-hawker merged commit 20f951e into CommunityToolkit:main Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header on TokenizingTextBox has creative interpretation of being a header (faulty rendering)
4 participants