-
-
Notifications
You must be signed in to change notification settings - Fork 320
feat: Form Related Changes in Add Exercise Screen #125
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
feat: Form Related Changes in Add Exercise Screen #125
Conversation
List<File> get excerciseImages => [..._excerciseImages]; | ||
final List<File> _excerciseImages = []; | ||
String? _name; | ||
String? _alternativeName; |
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.
the alternative names should be a string since some exercises can have several names
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.
It's already string!
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.
Sorry, I meant a list of strings
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.
Every string seperated by space is new name. Correct?
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.
Either space or newline, yes. I think newline is more obvious since some names can have whitespaces in them
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.
I need to think, how we can implement these.
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.
These part is still pending, but branch is merged?
String? _name; | ||
String? _alternativeName; | ||
String? _targetArea; | ||
List<String?>? _primaryMuscles = []; |
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.
wouldn't it be better to store this a a list of Muscles? Same goes to the other stuff we get from the form, it will be easier for us later when we need to do work with those objects later
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.
Currently DropDowns and MultiSelect widgets are designed to handle only Strings and not Object, that is the reason I kept it as string.
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.
Ah, ok. Can we store the object IDs and do a lookup when saving?
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.
that's possible , but then in UI we will only see ID and not name.
Instead we need to re-design the widgets.
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.
Showing only the IDs is not optimal 😄, I guess we will have to think of something
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.
Cool then we need to re-design the widgets,
I'll try to do them.
Form Changes
Please check that the PR fulfills these requirements