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

New control MaterialExpansionPanel #143

Merged
merged 11 commits into from
May 8, 2021
Merged

New control MaterialExpansionPanel #143

merged 11 commits into from
May 8, 2021

Conversation

orapps44
Copy link
Collaborator

Introduce Expansion Panel control (without animation).

Introduce new control Expansion Panel (No animation implemented yet).
Add Expansion Panel Colors & Brushes
Add button enable property
Chenge top padding
Fix expand height issue in design mode
Fix description bounds
remove extra blank line
Add MaterialExpansionPanel.cs
@leocb
Copy link
Owner

leocb commented Feb 20, 2021

Nice work! , I'll review it tomorrow!

Margin default value fix
Copy link
Owner

@leocb leocb left a comment

Choose a reason for hiding this comment

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

Please, see the review notes, most are just details and I'm nitpicking here. Overall, very good job!

Also, please add it on the example app, in the "Containers" tab (this is container.. right?)

And don't worry about adding animations on it for now, those are a pain to do on windows forms, specially if they make other components redraw (see drawer workaround mess I did). animation on the buttons would be nice tho.

MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
MaterialSkin/Controls/MaterialExpansionPanel.cs Outdated Show resolved Hide resolved
private const int _buttonPadding = 8;
private const int _expandcollapsbuttonsize = 24;
private const int _textHeaderHeight = 24;
private const int _headerHeightCollapse = 48;
Copy link
Owner

Choose a reason for hiding this comment

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

_headerHeightCollapse and _headerHeightExpand should be the same. The way they are now, the header is "dancing around" when changing states, and that's a big no no.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be only a misunderstanding due unappropriate constant name. Better name should be something like _controlHeightCollapse & _controlHeightExpand.
If not then please clarify a bit.

private const int _expandcollapsbuttonsize = 24;
private const int _textHeaderHeight = 24;
private const int _headerHeightCollapse = 48;
private const int _headerHeightExpand = 64;
Copy link
Owner

Choose a reason for hiding this comment

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

Also, I didn't verify this, but the component seems to have more padding then the other components, check the "padding and margins" values I use on other components, those should be constant trough the lib. I actually need to put them on a helper or something, but that's a refactor for another day, for now, just use the same values I do. I may just be crazy tho, so if I am wrong, just ignore this :P

Update following leocb review (thanks).
Some conditions where missing before to fire events.
Use MaterialButton control for validation
Add Button minimum width management
Fix ValidationButtonEnable default value
Fix Expand button color when disabled
@leocb
Copy link
Owner

leocb commented May 7, 2021

Conflicts, they just keep on poping up huh

@leocb leocb added this to the 2.2 milestone May 7, 2021
@orapps44 orapps44 merged commit 53544cb into leocb:master May 8, 2021
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.

2 participants