@sea2709 Please don't apologise, the work you have done here is fantastic and is going to make a huge impact to the UX and to our demo at DrupalCon.
I think I've resolved the Cypress issue and I've given some small feedback on your code but it's really great stuff!
wim leers → credited jessebaker → .
Duplicate of 🐛 Numeric field input crashes contextual settings panel Active
RE #12 it looks like the test is still flakey but it's flakey in a slightly different way now! I think that counts as progress.
I've just fixed conflicts so this branch now has the fixes that were added in 📌 Redux Sync all single-value types in the SDC test all props form Fixed but I'm not able to look into this right now as other issues have priority.
jessebaker → created an issue.
jessebaker → created an issue.
jessebaker → created an issue.
Approved and merged!
jessebaker → made their first commit to this issue’s fork.
Merged! This is a really nice improvement all round.
Closing as dupe of ✨ Show context menu on right clicking components in the Layers panel Needs review
Approved - and love that this resolves the flakey components-slots test!
-> @wim leers for backend review
🙏 merged!
wim leers → credited jessebaker → .
I've approved the MR. I actually like the removal of the delay on showing the tooltip from a UX perspective too - in this instance it makes the list feel more responsive and I don't think it's too annoying/jarring for a user that it just pops up instantly. Nice.
jessebaker → made their first commit to this issue’s fork.
I've done some digging and Site Studio originally used to load the Media Library in an iFrame but had to resort to loading it directly on the page with their own custom CSS because it didn't play nicely when in an iFrame. I think there were several issues but the last of many smaller issues was an bug where after using the filter controls while Media Library was in an iFrame it became impossible to select an image.
This is not to say loading it in an iFrame is not possible or not the correct solution in the long term, but more to add a caveat that loading it in an iFrame is certainly not a quick win - and certainly not something I suspect we can achieve in time for the demo unless someone outside of Acquia is able to pick this up and bring it to completion.
Thanks all. I've merged a fix. I also verified that (at least on MacOS) the fix does not prevent a11y screen readers from working with the announce.js functionality
Discussed with @bnjmnm - the announce div is included as part of a longer term plan to improve a11y so it's correct that we are loading it on the page. For now I'm going to just set the body to have overflow: hidden and be done with it.
There is also 🐛 The preview container height will overflow outside the canvas Needs review - perhaps that issue could be used to track/develop a temporary solution to @balintbrews's concern #8 ahead of the demo date.
This will ultimately be solved by ✨ [later phase] Total canvas size should be dynamic based on browser viewport size Needs review but we will need a solution in time for the Drupalcon BCN demo.
@soaratul Test in pseudo code as discussed:
- Open page
- Click first header
- Validate that it has a required attribute
- Remove the value from the field
- Ensure the preview does NOT change
- With the value still blank (invalid) ensure further interaction is "locked" e.g...
- - ensure you can't select a different component
- - ensure you can't drag a new component on
- - ensure you can't rearrange components
- - ensure you can't delete a component
- Put a new value in
- Ensure the new value shows in the preview
- Ensure you now CAN select a different component, drag, rearrange, delete etc.
I'm afraid I have quite a bit of feedback!
The zoom commands (CMD/CTRL and + CMD/CTRL and -) are currently zooming the whole interface due to default browser behavior.
This seems to be fixed but the new test introduced in this MR is not actually testing that. TBC if it's even possible to test that the default browser behaviour is being prevented in a Cypress test!
There seems to a bug that means that pinch-zooming on a trackpad on Mac can also sometimes zoom the whole browser, this should also be addressed here.
This is not addressed yet. (mouse moving between iframe and parent during the zoom action causes it) - lets raise another issue for this specifically.
The MR is adding the ability to press the 0 key to reset the zoom but
- If you focus in the iframe and then press (specifically) NUMPAD0 it doesn't pass that event up to the parent. (The other 0 key works)
- The new test introduced does not test for the above usecase.
If you zoom to an irregular zoom amount (like 83% instead of one of the zoom amounts listed in the dropdown) using pinch zoom (or ctrl + mouse wheel) and then press +, the zoom snaps to 25%. I don't know if this was introduced in this MR.
Tested, and code reviewed. Merging!
jessebaker → made their first commit to this issue’s fork.
I just had to make one small change to the tests - each it() block should not depend on the previous test having run successfully to pass. They should be independent. I moved the 3 new tests into a single it block. See https://docs.cypress.io/guides/references/best-practices#Having-Tests-Re...
@shyam_bhatt You're code change does seem to resolve the issue, but I fear it is just treating a symptom and not addressing the cause.
This issue seems to have been introduced in https://www.drupal.org/project/experience_builder/issues/3454173 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Fixed
The addition of - system/base in experience_builder.libraries.yml means the /xb page is now loading extra JS libraries. One of which is announce.js. The code in announce.js add a new div to the page at the bottom of the body with a height/width of 1px that sits below the experience builder div and is what is causing the scrollbar.
Assigning to @bnjmnm to take a look.
Changing the resolution of the viewports available in the demo is fairly easy but requires manually tweaking the canvas size and possibly updating some tests (tbc). It would be good to definitely all agree on which viewports we want before implementing.
I agree that 1024 is very small for a desktop/laptop these days but it's still a common size for iPads/tablets.
I think designing with a 1080p viewport will be challenging on anything less than a 4k screen and I'm not sure how well it would demo at that resolution.
My suggestions
For the demo:
Desktop: 1280*768
Mobile: 400*768
Medium term:
Introducing a 3rd viewport and renaming the existing one to tablet:
Desktop 1920*1080
Tablet 1024*768
Mobile: 400*768
Longer term:
Number of breakpoint viewports and each of their resolutions will be user defined/configurable.
Also, can I please clarify how the decision was reached to not build the demo SDC's responsively (to work at all resolutions)? My guess is that it was just done in the interest of speeding up development but I just want to make sure I understand correctly in case I'm making a wrong assumption there and there is not some other (technical or intent related) reason that I should be considering in future.
🐛 Adding a component with slots does not register the slots as children Fixed has now landed
Updated the Proposed resolution in the issue summary.
@omkar-pd I attempted to get the tests passing but I have run out of time, unassigning and setting to Needs work for your (or someone else) to take another look.
@omkar-pd I don't think that scrollbar on the right should be there at all! Is the screenshot taken on Windows?
@wim leers yes! The title was written too hastily (by me) in my attempt to get a fork up and running to attempt to fix it and then you jumped on it before I could rename it to be more specific, so thank you!
It looks like my attempted fix for the Cypress test has passed at least once. I'm going to run it a few more times to be a bit more reassured that it's truly solved.
jessebaker → created an issue.
@boulaffasae Your description is accurate of the issue I've been addressing here and I believe that this is now fixed if you would like to try and re-test!
jessebaker → made their first commit to this issue’s fork.
This is not passing the tests yet. Please ensure the tests pass before setting status to Needs review.
Merged! Great work everyone, thank you!
Merged! Thank you everyone
jessebaker → made their first commit to this issue’s fork.
🎉 Merged!
Merged!
Closing as duplicate of 🐛 Adding a component with slots does not register the slots as children Fixed
Merged, this was just left-over debug code. Thank you!
jessebaker → made their first commit to this issue’s fork.
jessebaker → made their first commit to this issue’s fork.
@tedbow I have fixed 2/3 of the failing tests. The 3rd test seems to be failing on something specific to this branch so I believe it’s a legitimate bug.
After choosing an image from the Media Library there is an error when making a request to /api/preview/node/1
as follows:
The website encountered an unexpected error. Try again later.<br>
<br>
<em class="placeholder">Drupal\Core\Render\Component\Exception\InvalidComponentException</em>
: [image.src] Does not match the regex pattern ^(/|http).*\.(png|gif|jpg|jpeg|webp)(\?.*)?(#.*)?$ in <em class="placeholder">Drupal\Core\Theme\Component\ComponentValidator->validateProps()</em>
(line <em class="placeholder">203</em>
of <em class="placeholder">core/lib/Drupal/Core/Theme/Component/ComponentValidator.php</em>
). <pre class="backtrace">Drupal\Core\Template\ComponentsTwigExtension->doValidateProps(Array, 'experience_builder:image ') (Line: 103)
Drupal\Core\Template\ComponentsTwigExtension->validateProps(Array, 'experience_builder:image ') (Line: 42)
__TwigTemplate_331ff7e5a20fda144110fb9d0e1512e8->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array, Array) (Line: 123)
__TwigTemplate_a529a4e6ed6302d18950f54e7bf80640___162390326->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 40)
__TwigTemplate_a529a4e6ed6302d18950f54e7bf80640->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 335)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 234)
Drupal\Core\Template\TwigEnvironment->renderInline('{# inline_template_start #}{# This template was dynamically generated by single-directory components #}
{% embed 'experience_builder:image '%}
{% endembed %}
', Array) (Line: 54)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)
call_user_func_array(Array, Array) (Line: 107)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725 ', 'exception ', 'Drupal\Core\Render\Element\RenderCallbackInterface ') (Line: 825)
Drupal\Core\Render\Renderer->doCallback('#pre_render ', Array, Array) (Line: 387)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 203)
Drupal\Core\Render\Renderer->render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html ', NULL, 1) (Line: 147)
__TwigTemplate_8d68d944fd3cb9a2ce56e894397a334b___1318261257->block_column_two(Array, Array) (Line: 430)
Twig\Template->yieldBlock('column_two ', Array, Array, 1) (Line: 193)
Twig\Template->renderBlock('column_two ', Array, Array) (Line: 66)
__TwigTemplate_99ea5cc98d60103a120c7f4dc604a8ad->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array, Array) (Line: 125)
__TwigTemplate_8d68d944fd3cb9a2ce56e894397a334b___1318261257->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 40)
__TwigTemplate_8d68d944fd3cb9a2ce56e894397a334b->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 335)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 234)
Drupal\Core\Template\TwigEnvironment->renderInline('{# inline_template_start #}{# This template was dynamically generated by single-directory components #}
{% embed 'experience_builder:two_column '%}
{% block column_one %}
{{ column_one }}
{% endblock %}
{% block column_two %}
{{ column_two }}
{% endblock %}
{% endembed %}
', Array) (Line: 54)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)
call_user_func_array(Array, Array) (Line: 107)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725 ', 'exception ', 'Drupal\Core\Render\Element\RenderCallbackInterface ') (Line: 825)
Drupal\Core\Render\Renderer->doCallback('#pre_render ', Array, Array) (Line: 387)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 459)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 203)
Drupal\Core\Render\Renderer->render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html ', NULL, 1) (Line: 80)
__TwigTemplate_77fd3c900ebe4612a401cff4784a3799->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 335)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 33)
twig_render_template('core/modules/system/templates/page.html.twig ', Array) (Line: 348)
Drupal\Core\Theme\ThemeManager->render('page ', Array) (Line: 446)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 203)
Drupal\Core\Render\Renderer->render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html ', NULL, 1) (Line: 81)
__TwigTemplate_3a9bda3284efa427aa1dce14c22e6c78->doDisplay(Array, Array) (Line: 360)
Twig\Template->yield(Array) (Line: 335)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 33)
twig_render_template('core/modules/system/templates/html.html.twig ', Array) (Line: 348)
Drupal\Core\Theme\ThemeManager->render('html ', Array) (Line: 446)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 203)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 108)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 107)
Drupal\Core\Render\Renderer->renderRoot(Array) (Line: 66)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, '', 'page ', Array) (Line: 76)
Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage(Array, '', 'page ') (Line: 388)
Drupal\experience_builder\Controller\SdcController->preview(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 37)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
require('/Users/jesse.baker/Workspace/drupal11/web/index.php ') (Line: 71)
</pre>
jessebaker → made their first commit to this issue’s fork.
balintbrews → credited jessebaker → .
jessebaker → created an issue.
jessebaker → made their first commit to this issue’s fork.
All fixed! Hooray!
Follow up to address that last nice to have is created here: 📌 Improve/Refactor replaceUUIDsAndUpdateModel() and insertMultipleNodes() Active .
jessebaker → created an issue.
wim leers → credited jessebaker → .
jessebaker → created an issue.
jessebaker → created an issue.
@sea2709 the work you have done in https://git.drupalcode.org/project/experience_builder/-/merge_requests/241 seems to be a good approach to me. As you mentioned it covers more use cases (like loading in the component's CSS and JS) than what we have now.
However I think more work is required before it can be merged. These issues do seem quite challenging to solve, but ultimately are required for a good UX.
1) On hover there is a flash/flicker where the tooltip container (the div with the dropshadow) shows but the iFrame has not yet loaded. Can this be improved to look smoother.
2) The scaling seems to be wrong. Ideally a small button would display full size but a large Hero component would be scaled to a maximum width to fit inside the tooltip
3) the height of the tooltip is fixed and a lot taller than it needs to be.
(see above screenshot)
I don't think this is reproducible on head now that 📌 Improve the page hierarchy display Active just got merged in? Can someone who was able to reproduce this before please try again on 0.x?
jessebaker → created an issue.
I've just approved this from the /ui code perspective. There is obviously more to be done here, but I see no reason not to go ahead and merge this as a significant step forward.
@Wim Leers Would it be safe to change the title of this ticket to "Create Cypress E2E test to ensure that /enum fields in the components prop form work" or is there some unique aspect of "variants" that specifically needs to be tested here?
Minor thing, but as per https://www.drupal.org/project/experience_builder/issues/3459249 📌 Allow opening the contextual menu by right clicking a component Needs work it should be possible to right click directly on a component, not just its name label.
I've given @balintbrews as much context about this issue as I can so hopefully he can assist with anything you need next week.
@balintbrews - has your work on the side panel made this ticket out dated?
@balintbrews I'm sorry I didn't get to this before my time off! I think the best course of action here is to merge what has been done because it is a big improvement and then open a follow up ticket to further address the issue (now instead of jumping the view, it simply stops the panning from working).
RE #7
I'm wondering if we could be opinionated about section templates and as a UX enhancement, only allow adding them to the main level.
I don't think this would be beneficial for two reasons
1) If the issue is that it is difficult with the current UI to accurately place the thing you are drag/dropping into nested slots then we will still need to solve that problem for components so just preventing sections from working in that way would just be a sticky plaster
2) Ultimately I don't think we need to restrict users in that way - As an example, a given user's particular page layout may be based around having a large layout component that has a slot for all the page content and they would want to place their sections inside that larger component.
Wim Leers → credited jessebaker → .
jessebaker → made their first commit to this issue’s fork.
jessebaker → created an issue.
jessebaker → created an issue.
In Site Studio's Visual Page Builder the target slot (or Dropzone to use their terminology) is highlighted with a dotted border when you are dragging and hovering over the slot. I think we should have something similar.
RE #8 Have you tried a cache clear since checking out this branch? I think that is what I did to resolve the same issue.
Thanks for pushing forward with this. I’ve added some design/UI feedback to the MR. I’ve not yet done a code review. I will leave that until the UI is looking correct unless you have any specific parts of the code that you would like feedback on before you continue.
Wim Leers → credited jessebaker → .
jessebaker → made their first commit to this issue’s fork.