-
Notifications
You must be signed in to change notification settings - Fork 6
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
Enhancements #124
Enhancements #124
Conversation
since we're setting the default value of preview to true, we can't return return the placeholder image early since it prevents the edit controls from rendering
@dkotter @jeffpaul I'm stuck making the tests work after this change - by setting the preview to default to true, the winamp canvas element is blocking cypress from clicking the publish button. The error message suggests forcing the click, but the click in question is handled upstream by the cypress utils. Additionally, this could be hinting at an accessibility issue that might make us reconsider showing the preview by default. Any thoughts here?
|
A couple concerns I have:
I think we need to solve the fixed positioning issues if we want to have the preview on by default. Otherwise it's pretty annoying, especially if you have additional content in the post besides just this block, as it ends up covering everything. |
@dkotter I took a pass at making what I feel is a pretty good UX solution here. The approach is a little heavy handed, but the webamp library is pretty limited in terms of what we're trying to accomplish. The updates now move the webamp component inside the block markup and positions it neatly, which a.) activates the block when the user interacts with the winamp component and b.) looks a lot nicer than the previous position which was just randomly floating on the page. Check out the gif in the PR description and let me know what you think. cc @jeffpaul |
This looks good to me! Nice work here @psorensen |
Description of the Change
Closes #119
How to test the Change
Changelog Entry
Credits
Props @psorensen
Checklist: