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

Adding control properties API #49

Merged
merged 4 commits into from
Apr 21, 2021
Merged

Adding control properties API #49

merged 4 commits into from
Apr 21, 2021

Conversation

erickzanardo
Copy link
Member

@erickzanardo erickzanardo commented Apr 20, 2021

This PR adds the control properties API.

Fixes #47

Gravacao.de.Tela.2021-04-20.as.15.27.10.mov

@erickzanardo erickzanardo requested a review from tyemy April 20, 2021 18:27
@erickzanardo erickzanardo mentioned this pull request Apr 20, 2021
@erickzanardo erickzanardo requested a review from tyemy April 21, 2021 00:57
errorColor: ctx.colorProperty(
'errorColor',
const Color(0xFFCC6941),
controlProperty: ControlProperty('type', MessageCardType.error),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could help if this property had a name related with “visibility” or something like that

Copy link
Contributor

@jeffs-sf jeffs-sf left a comment

Choose a reason for hiding this comment

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

As the author of #47 this does exactly what I was looking for, thank you! There's some room for expansion later (maybe hiding if it doesn't match the type verses including if it does), but this helps tremendously.

There's one missing property that doesn't currently respect the ControlProperty but allows the consumer to define it (TextProperty) and some minor grammar suggestions. I added support for it locally to verify that passing in that ControlProperty to the Property works.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example/lib/widgets/message_card.dart Outdated Show resolved Hide resolved
Comment on lines +26 to +28
final List<Widget> children = [];

for (var entry in widget.currentChapter.ctx.properties.entries) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: we could break this out into a method

Comment on lines 39 to 40
String textProperty(String name, String defaultValue,
{ControlProperty? controlProperty}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

chore (blocking): we need to utilize the controlProperty here and pass it to the Property like we added for the other property types.
https://github.com/erickzanardo/dashbook/blob/0de95e01ae6898d27deb4e5f0b7fedcae763fad1/lib/src/story.dart#L33-L34

      final property = Property<String>(name, defaultValue,
          controlProperty: controlProperty);
      properties[name] = property;

erickzanardo and others added 2 commits April 21, 2021 14:27
Co-authored-by: Jeff Scaturro <53348890+jeffs-sf@users.noreply.github.com>
@erickzanardo
Copy link
Member Author

Thanks for the review @tyemy and @jeffs-sf I have applied the suggestions.

@erickzanardo erickzanardo merged commit 35a7bf1 into main Apr 21, 2021
@erickzanardo erickzanardo deleted the erick.control-property branch April 21, 2021 17:40
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.

Dynamic Properties
3 participants