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

Infinite Canvas dropdown font selector instead of textbox #3211

Closed
wants to merge 14 commits into from

Conversation

salmanmkc
Copy link
Contributor

@salmanmkc salmanmkc commented Apr 1, 2020

Fixes

No issue that I know of, just a change

PR Type

What kind of change does this PR introduce?
A UI change

What is the current behavior?

You have to enter a font size with text in the textbox, I used the standard font sizes that are given in Microsoft Office PowerPoint (increments from 8 to 96).

What is the new behavior?

User can select the font size themselves now

Screenshots

Before

image
Textbox to change the font size

After

image
Showing the ComboBox collapsed

image
Expanded ComboBox

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)
  • 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

@ghost
Copy link

ghost commented Apr 1, 2020

Thanks salmanmkc 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 assigned azchohfi Apr 1, 2020
Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

@salmanmkc Thanks for the PR! Just delete the duplicate size 48. Everything else looks good!

image

@ghost
Copy link

ghost commented Apr 8, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@salmanmkc
Copy link
Contributor Author

@salmanmkc Thanks for the PR! Just delete the duplicate size 48. Everything else looks good!

image

Thanks for spotting that! Fixed! 😊

Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

Thanks, @salmanmkc. Looks good now 🎉

@michael-hawker michael-hawker added the for-review 📖 To evaluate and validate the Issues or PR label Apr 14, 2020
@michael-hawker michael-hawker added this to the 6.1 milestone May 18, 2020
@michael-hawker
Copy link
Member

@azchohfi @skendrot thoughts on this since we're changing the template, we probably want this in 7.0 and not 6.1, eh?

@azchohfi
Copy link
Contributor

This is a breaking change. It should go to 7.0, not 6.1.

@ghost
Copy link

ghost commented Sep 2, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment.

…s.xaml

Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>
@ghost
Copy link

ghost commented Sep 11, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@ghost ghost removed the no-recent-activity 📉 Open Issues that require attention label Sep 11, 2020
@salmanmkc
Copy link
Contributor Author

I committed your change you made @michael-hawker, did I need to look into conditional properties?

@salmanmkc
Copy link
Contributor Author

oh I see for isEditable, I will try look into that and utilise strings there

@michael-hawker
Copy link
Member

@salmanmkc once #3478 is merged you won't have to worry about a conditional here. So once that gets merged, let's update this PR.

@ghost ghost removed the needs attention 👋 label Sep 18, 2020
@michael-hawker michael-hawker marked this pull request as draft September 18, 2020 20:36
@salmanmkc
Copy link
Contributor Author

Build seems to be failing now, not sure what the issue is

@salmanmkc
Copy link
Contributor Author

Same with the master branch, I think it's just my user login

@michael-hawker
Copy link
Member

@salmanmkc last build status seemed fine to me, updating here. What are you seeing?

@Kyaa-dost
Copy link
Contributor

@salmanmkc update on this one?

@Kyaa-dost
Copy link
Contributor

@salmanmkc the build is running fine. Can you please check one more time? 😊

@salmanmkc
Copy link
Contributor Author

@salmanmkc the build is running fine. Can you please check one more time? 😊

Yes, will have a look now

@Rosuavio
Copy link
Contributor

Hi @salmanmkc, I'm looking to review this PR, do you mind resolving the conflicts? Also if you can add back in the IsEditable prop then we might be able to get this in soon.

@Kyaa-dost
Copy link
Contributor

@salmanmkc is there any update on this?

@michael-hawker michael-hawker modified the milestones: 7.0, 7.1 Mar 1, 2021
@michael-hawker
Copy link
Member

Moving to 7.1

ghost pushed a commit that referenced this pull request Apr 20, 2021
<!-- 🚨 Please Do Not skip any instructions and information mentioned below as they are all required and essential to evaluate and test the PR. By fulfilling all the required information you will be able to reduce the volume of questions and most likely help merge the PR faster 🚨 -->

<!-- 📝 It is preferred if you keep the "☑️ Allow edits by maintainers" checked in the Pull Request Template as it increases collaboration with the Toolkit maintainers by permitting commits to your PR branch (only) created from your fork.  This can let us quickly make fixes for minor typos or forgotten StyleCop issues during review without needing to wait on you doing extra work. Let us help you help us! 🎉 -->


## Continued off of #3211

<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->
User can now input a font size via typed text and not only selection from out provided sizes.

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
- Feature
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
You have to enter a font size with text in the textbox, I used the standard font sizes that are given in Microsoft Office PowerPoint (increments from 8 to 96).

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
User can select the font size themselves now


## PR Checklist

Please check if your PR fulfills the following requirements:

- [ ] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [ ] Sample in sample app has been added / updated (for bug fixes / features)
    - [ ] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)
- [ ] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/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

<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. 
     Please note that breaking changes are likely to be rejected within minor release cycles or held until major versions. -->


## Other information
@michael-hawker
Copy link
Member

Closing this as we finalized this in #3914.

@michael-hawker michael-hawker modified the milestones: 7.1, 7.0.2 May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for-review 📖 To evaluate and validate the Issues or PR improvements ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants