- Issue created by @kunalkursija
- 🇮🇳India anjali rathod India
I am facing issue setting up the responsive preview module. Attaching the screen recording video for reference.
- 🇮🇳India roshni upadhyay
Hi @lauriii,
I encountered an issue while working with the responsive_preview module. The error was as follows:
Error: Call to undefined function drupal_common_theme() in responsive_preview_theme() (line 40 of modules/contrib/responsive_preview/responsive_preview.module)
I resolved it by modifying the code in the responsive_preview_theme() function. Could you please confirm whether this should be addressed as a separate issue, or can we include the fix under the scope of the current issue?
Additionally, I need clarification regarding the default dimensions for the device icons. What dimensions should we use for both devices? Please provide the recommended width and height values to standardize the implementation.
- 🇮🇳India roshni upadhyay
I would like to clarify that I am still facing the issue. In my previous comment, I mentioned that it was working, but that was due to a version mismatch. The issue persists as follows:
When the Responsive Preview block is placed, the buttons are displayed but do not function as expected.
If the Navigation module is uninstalled, the Responsive Preview list displays correctly in the toolbar and works perfectly. - 🇮🇳India roshni upadhyay
The Responsive Preview block was not functioning as expected due to the missing #block-responsivepreviewcontrols ID when the block was placed. After adding the required ID, the block started working as intended. With this issue resolved, I am now unblocked and able to continue working on it.
- Merge request !22Issue# 3489112: Integrate responsive preview with Navigation top bar → (Open) created by Unnamed author
- 🇪🇸Spain plopesc Valladolid
@roshni upadhyay Thank you for your work on this one.
After reading the code, I have some ideas to make it a bit more generic:
* We could create the navigation top bar as a submodule, or even a new contrib module that depends on this one and navigation. Let's work on a MR for this one for now.
* Add a 3rd party setting to the config entity as part of this submodule to "Show in Top Bar" the responsive_preview entity
* Instead of hardcoding the types inResponsiveIcons
class, load the items whose 3rd party setting is defined to be shown in the top bar
* Think in a way of how to display those options different from "mobile" & "desktop". That could be part of a follow-up issue - 🇪🇸Spain plopesc Valladolid
Another approach could be to make it fully hardcoded, and do not make use of the config entities, and use the original module as an API to provide the logic to switch between devices, but not crating new entities, nor using them.
This 2nd approach could be simpler to implement at this point, but we will probably want something more generic in the long term.
- 🇫🇮Finland lauriii Finland
Discussed with @plopesc and @roshni upadhyay to align on the approach. We agreed that we will continue with the approach proposed in #11. What we will do is:
- Create a new submodule specifically for Drupal CMS (and Experience Builder)
- For now, the submodule will not be using the
Device
config entity but we will hard code those values in theDrupal\navigation\TopBarItemBase
implementation which will allow us to provide a good experience consistently - We will avoid making changes to the main module for now so that we can ship the submodule in a separate project if necessary
- In future, we might integrate the device settings with Experience Builder to automatically define them based on what has been configured there.
- 🇪🇸Spain plopesc Valladolid
Changes look great and goes in the right direction.
Added some comments in the MR that might need to be addressed.
- Assigned to roshni upadhyay
- Status changed to Needs review
about 1 month ago 4:39am 22 January 2025 - 🇪🇸Spain plopesc Valladolid
Made a couple of minor adjustments, but overall, I think this is ready for final review.
- 🇫🇮Finland lauriii Finland
This looks pretty good based on the functionality and code has been reviewed by @plopesc.
I think an important follow-up to this would be to render the responsive preview as an anonymous user. That might lead into some complications so it's better to leave that to another issue.
- 🇪🇸Spain plopesc Valladolid
Created follow-up 📌 Preview displays include Navigation Toolbars Active
- First commit to issue fork.
- 🇮🇳India rajeshreeputra Pune
Rebased, will merge this and wait for the follow-up 📌 Preview displays include Navigation Toolbars Active .