Obojobo
Obojobo copied to clipboard
Updated iframe properties dialog
Fixes #1536
A sneak peak at our new iframe properties modal:
I numbered the issues on Zac's last comment so they are easier to address and talk about.
(1) Done.
(2) Done.
(3) Done.
(4) Done.
(5) Done. I just need some feedback on the purple borders. Any thoughts about keeping the purple borders all the time or only on hover? Right now, I fixed it to be only on hover.
(6) Done.
(7) Done.
(8) Done.
(9) Done.
(10) Done. Applied the mixins and adapted inputs and selects to have the same border color.
(11) I haven't found any background colors set on iframes in the viewer. Could you send me any screenshots? :)
(12) Done.
(13) Proposing this one: I thought about disabling the button to go from the first modal to the second if the src
typed is not valid. Any thoughts?
Finished applying the changes Zac requested. This PR is testable/reviewable again.
The tests that are not passing are some uncovered React functions I had to write in order to control more complicated hover/click actions (Change n#5 above). I'm just waiting to get your feedback on the current hover/click states I implemented before writing new tests to cover those new functions.
Looks good so far! Only thing I noticed is that setting it to "Embedded webpage" leaves out the Border setting when setting everything to true. Everything else seems to be working as expected with my testing. Good work!
For your proposal with (13), I think that it could be useful since it'd help prevent accidentally giving an invalid src and not realizing it. I don't think there are any cases where an invalid src would be intentional, but if they do exist then an extra warning message similar to the missing src message might be a better option.
Also, #1875 was recently merged in and removes the leading comma from the controls
strings, so those should be safe to remove in the code now.
Thanks for all the suggestions so far everyone!
New changes include:
- Addressed the last changes that @jpeterson976 requested.
- I also found a handful of bugs with how
content types
andcontrols
were handled. For that, I merged theborder
property into thecontrols
property, allowing me to refactor a lot of code and better handle how the entirecontrols
state is managed. - I introduced a new property to the iframe context, called
controlsChanged
to better address Zac's n#8 issue above. I was facing a bit of a boolean dilemma setting the controls to have specific properties depending on their iframe's content type (because we would also need to know when users created iframes, turned off all controls, and closed them; created iframes, left the controls with their default options, and closed them; and all kinds of combinations of these user actions). This new prop solved this issue (apparently).
Let me know your thoughts :)
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep our backlog under control, but we thank you for your contributions.
These iframe options are really cool. I didn't find any issues with anything. I tested on different browsers and played around with the sizing and other settings.