-
Notifications
You must be signed in to change notification settings - Fork 58
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
Implement list settings #1619
Implement list settings #1619
Conversation
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.
Hey @SergioEstevao - Thanks for taking care of this feature! 🎉
I found some weird behavior while testing it on iOS:
- Changing settings won't always be reflected visually on the block
- At some point the block format would break
There's a gif showing this issue:
It's interesting that some times the visual changes would work according to the settings. For example:
- Create the ordered list with some elements.
- Change settings to reversed (It won't show the changes visually in the list).
- Go to HTML mode and back.
- The block is re-rendered with the correct list order (reversed).
- Select the block without making the keyboard active (tapping at the very edge).
- Select the block settings and change properties.
- It will refresh the block visually to reflect the changes ✅
- Select the block making the keyboard active and change some settings.
- It won't refresh the block visually ❌
One possible enhancement could be to show the numeric pad to set the Start Value setting.
I saw that TextControl
has a type
prop that is set to number
on this setting, but it's not being handled properly in our native TextControl
version. I think that if we pass the corresponding keyboardType
to the Cell component it should work.
I gave it a pass on Android but I think it's not ready yet, right?
@etoledom Thanks for the great review! Two great catches, I updated the code to address those issues, can you please give it another look? |
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.
Thank you @SergioEstevao for the update, it's working great now!
All the issues pointed out previously are now fixed 🎉
Not sure if we should wait for the Android implementation or we could have a platform check to ship on iOS ahead of time.
@marecar3 I tested the lists in Android and I see one bug, if you choose a reversed list without specifying a start value, the values are going from top value to 0. For example if you have 5 items we see:
While the correct should be:
|
# Conflicts: # gutenberg
Hey @SergioEstevao, thanks for the feedback. |
Hey @SergioEstevao, you can do another iteration of testing :) Thanks! |
@marecar3 Working great now! Thanks for this! Do you need to merge anything in Aztec Android before I merge this? |
# Conflicts: # react-native-aztec/ios/Cartfile # react-native-aztec/ios/Cartfile.resolved
Fixes #1304
To test:
Update release notes:
RELEASE-NOTES.txt
.