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 #1958

Merged
merged 71 commits into from
May 23, 2018
Merged

Conversation

IbraheemOsama
Copy link
Member

@IbraheemOsama IbraheemOsama commented Apr 8, 2018

Issue: #1572

PR Type

What kind of change does this PR introduce?
Infinite Canvas #1572

Feature

What is the current behavior?

What is the new behavior?

Infinite Canvas

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • 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

@michael-hawker
Copy link
Member

Here's the UWP OneNote text color for reference:

image

The 'A' isn't as narrow and it has the bigger block of color underneath it for the current color selection.

I don't know if we have to go so far as swapping out the entire color picker, but I think it can be tweaked for black to still show the color somehow, no?

For the other comment, there were two pieces:

  1. 'Touck Inking' typo in toolbar
  2. This was what it looked like before I saved (using pencil):

image

And after loading (its ballpoint pen):

image

@IbraheemOsama
Copy link
Member Author

@nmetulev @michael-hawker ping.
For enabling touch Ink in case pen is not supported I couldn't find an API (I didn't dig deep) but when I opened edge it was selected by default as well and I don't think I used it before.
I have another issue when click the bold/italic buttons the styles of the button doesn't work well with the theme !! should I restyle the whole toggleButton or maybe you have a workaround.

@michael-hawker
Copy link
Member

Some new issues I found:

  1. Was editing a textbox and then hit erase all but the textbox outline still remained:

image

I couldn't type until I clicked elsewhere. Probably should remove the current box and wait for the user to click and interact again.

  1. If I tab away from the font-size box, then I get a weird cursor artifact on the right:

image

  1. If I type letters in the font-size and then tab away, they get erased, but the current font-size isn't restored as the value.

  2. @WilliamABradley guess I need to make a feature request for the Text Toolbar to support a drop-down menu style, eh?

image

  1. Is there a way for the selected text to be readible still, it's just a big block currently:

image

  1. The cursor/dimensions are still off when making something bold (see above selection) until the box is deselected and interacted with again:

image

@IbraheemOsama
Copy link
Member Author

@michael-hawker thanks for your valuable input :) I will work on the issue except number 5 because it's a limitation in the textBox as you can't change the opacity of the selected text but you can change the color.

@IbraheemOsama
Copy link
Member Author

@michael-hawker @nmetulev ping.

@nmetulev
Copy link
Contributor

The only thing I noticed is that typing letters in the font-size is allowed and they don't get removed. I'd expect the previously good text size to be restored as soon as I type away. Or only allow digits to be entered.

I recommend you get the documentation and icon finished as soon as possible to give us time to review.

Other than that, looks awesome.

@IbraheemOsama
Copy link
Member Author

infinitecanvas
infinitecanvas

@nmetulev @michael-hawker ping

Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

Awesome


## Examples

The following sample demonstrates how to add Grid Splitter Control
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - should be InfiniteCanvas control

<Page ...
xmlns:controls="using:Microsoft.Toolkit.Uwp.UI.Controls"/>

<controls:InfiniteCanvas Name="canvas"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some Properties and Events in this syntax. This look .....

Copy link
Member Author

Choose a reason for hiding this comment

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

The default usage for the canvas is "no properties" that's why it is better this way :)

@IbraheemOsama
Copy link
Member Author

@michael-hawker ping :)

@azchohfi azchohfi self-requested a review May 22, 2018 22:29
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.

One small comment on the default color, then I'm good.

<Button.Flyout>
<Flyout Placement="Bottom">
<ColorPicker x:Name="CanvasTextBoxColorPicker"
Color="Red" />
Copy link
Member

Choose a reason for hiding this comment

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

Thought this was just for the sample app, not sure if we want Red text to be the default for all text on a default InifiniteCanvas.

Probably just best to set it back to black. This is more an issue I have with the ColorPicker control itself. I may file some feedback hub issues on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but the black color picker is really misleading. I think i will create a property so the user can change the default color :)
lets leave it at developer's side 😈

@azchohfi azchohfi merged commit 3dc89b9 into CommunityToolkit:master May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants