-
-
Notifications
You must be signed in to change notification settings - Fork 256
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
Basic flex support #1021
Basic flex support #1021
Conversation
Thank you for your contribution. Do you have screenshots of HTML with flex and their rendering? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look, I have some comments. We also need some unit tests to cover the basics.
case 'div': | ||
final displayExpressions = meta.element.styles.where((element) => element.property == 'display'); | ||
final displayExpression = displayExpressions.isNotEmpty ? displayExpressions.first : null; | ||
final display = displayExpression?.term ?? 'block'; | ||
if (display == 'flex') { | ||
meta.register(_flexOp(meta)); | ||
} | ||
|
||
meta[kCssDisplay] = kCssDisplayBlock; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flex is supported for all kind of tags, not just DIV
right? I think we should handle flex for all of them in parseStyleDisplay
.
@@ -1150,14 +1158,103 @@ class WidgetFactory { | |||
return baseUrl.resolveUri(uri).toString(); | |||
} | |||
|
|||
/// Builds custom widget for div elements with display: flex from [meta] | |||
BuildOp _flexOp(BuildMetadata meta) { | |||
return BuildOp( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This op should be in its own file I think, probably packages/core/lib/src/internal/ops/flex.dart
. Because of the complexity, flex implementation will grow to support all kind of use cases, the factory file is already too long.
|
||
if (value != null) { | ||
switch (element.property) { | ||
case 'flex-direction': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repo usually defines constants for inline style key & values. Like this:
const kCssBoxSizing = 'box-sizing';
const kCssBoxSizingContentBox = 'content-box';
const kCssBoxSizingBorderBox = 'border-box';
onTree: (meta, tree) { | ||
_anchorRegistry.register(id, anchor); | ||
tree.registerAnchor(anchor); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these lines being removed? I think the anchor logic won't work without them?
That you for the review @daohoangson, I mode the review changes and added tests for basic usage, horizontal alignments, vertical alignments and flex directions. Let me know if there is anything else from preventing this to be merged into the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR also needs conflict resolve to merge into master
.
Conflicts have been resolved and flex has been converted to use new syntax. |
Added an initial support for very simple flex divs. This does not support all flex features, instead it allows some really basic flex features like alignments, justifications and directions.
Related to #877