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

Expander Control #100

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Expander Control #100

wants to merge 23 commits into from

Conversation

kat-y
Copy link
Contributor

@kat-y kat-y commented Oct 29, 2020

For issue 3279

Some expected changes to be added before this will be merged

active/Expander/Expander.md Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
@kat-y kat-y added the question Further information is requested label Oct 29, 2020
active/Expander/Expander.md Outdated Show resolved Hide resolved
@kat-y kat-y removed the question Further information is requested label Dec 2, 2020
@ranjeshj ranjeshj requested review from ejserna and alexhinz January 9, 2021 18:07
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
@adambarlow adambarlow removed the request for review from a team January 27, 2021 16:47
active/Expander/Expander.md Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved

XAML
~~~~
<muxc:Expander x:Name="Expander2" Header="This is in the header">
Copy link
Member

Choose a reason for hiding this comment

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

Presumably, I could also put content in the Header?

<muxc:Expander x:Name="Expander2">
    <muxc:Expander.Header>
        <Button>Button in the header</Button>
    </muxc:Expander.Header>
    <Button>Button in the content</Button>
</muxc:Expander>

Copy link

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify this, I'll be updating the examples so the second one has a ToggleButton in the header as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to have a toggle button in the header, the header is already a toggle button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By this I mean putting a ToggleButton inside the Header as part of an example showing controls in the Header and Content.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some other interactive control would help. A toggle within a toggle creates some confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend: Just some interactive content other than a ToggleButton.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a TextBox or RichEditBox (a scenario might be "Enter your password" with the dropdown showing a "Forgot password button" or something like that)?

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Great idea @oldnewthing, that's a very good example; @kat-y I think we should use that.

| ExpanderChevronMargin | Chevron margin thickness|
| ExpanderChevronGlyph | Chevron glyph|
| ExpanderChevronSize | Chevron size|
| ExpanderPopinVerticalOffset | vertical offset for animation|
Copy link
Member

Choose a reason for hiding this comment

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

How do I style the background for the header and content separately? Which one does the Background property apply to? Similarly for font properties and Foreground.

Is this where HeaderTemplate enters the picture?

Or is this a case where you have to edit the template, and we expect most people to accept the default background and font?

Copy link

Choose a reason for hiding this comment

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

I think the HeaderTemplate can be used to style the header, but @alexhinz knows more about this than me.

Copy link

Choose a reason for hiding this comment

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

Header, HeaderTemplate, and HeaderTemplateSelector are functionally analogous to Content, ContentTemplate, and ContentTemplateSelector, and they apply to the area of Expander that's to the left of the chevron. So because they don't span the entire border-to-border (inclusive) area of the header, they're likely not a good fit for that kind of styling. They also would not be aware of any Expander states, like if it's expanded, if a dev would like to change colors based on them.

So devs could retemplate to style the header and background areas separately. Adding in additional lightweight styling properties would help avoid the need to retemplate.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does Expander.Background apply to? Both the header and the content?

Other controls have a Background resource, should there be one for Expander too?

Copy link
Member

Choose a reason for hiding this comment

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

I think lightweight styling properties would be popular.

  • To convey state. E.g. "Tuesday has slots available, Wednesday is full, and Thursday is oversubscribed."
  • To color-code the Expanders. E.g., Errors in red, Warnings in yellow.
  • Everybody wants their colors to match their brand.

Copy link
Contributor Author

@kat-y kat-y Feb 9, 2021

Choose a reason for hiding this comment

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

Re @MikeHillberg question: Following the latest Expander PR merge the Background property now does propagates to the Header (while the Foreground, Fontsize, and FontFamily propagate to the Content).

As an example, the following code results in an Expander where the entire header area (including 'under' the chevron) is Cyan, with default text styling, but the content area has default background color with text that is pink and size 25 Times New Roman font. (If there's a way to add pictures to a PR thread let me know!). I'll update this section to reflect the current behavior for now.

<controls:Expander x:Name="Expander1" Header="This is in the header" Content="This is in the content" FontSize ="25" FontFamily="Times New Roman" Foreground="Magenta" Background="Cyan"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion: You could set the background of the Header using the Background property, and the background of the Content within the content itself.

Recommend: Maybe show an example?

active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Show resolved Hide resolved
active/Expander/Expander.md Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
active/Expander/Expander.md Outdated Show resolved Hide resolved
@Jay-o-Way
Copy link

Is this still an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants