Skip to content
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

Add notification when layouts are not available #7530

Merged
merged 6 commits into from Mar 12, 2024

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Mar 7, 2024

Resolves #7391

Proposed Changes

  • Display warning when there are no available layouts.

Testing Instructions

  1. Install and activate Divi. Enable Divi Builder for course and lesson post types.
  2. Go to Courses and create a new course.
  3. Make sure you see "No layouts available for this theme." on the layouts step.
  4. Go to Lessons and create a new lesson.
  5. Make sure you see "No layouts available for this theme. on the layouts step.
  6. Now, activate a few other themes (Course, 2024, Astra) and check you don't see the warning when you see layouts.

New/Updated Hooks

  • sensei.editorWizard.noLayoutsWarning - Filter no layouts message.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

Copy link

github-actions bot commented Mar 7, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Mar 7, 2024

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit 6e4a751 and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
admin/editor-wizard/index.js 13 kB +129 B ( +1.01% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.94%. Comparing base (229d03b) to head (6e4a751).
Report is 133 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7530      +/-   ##
============================================
+ Coverage     51.57%   51.94%   +0.37%     
- Complexity    11235    11267      +32     
============================================
  Files           622      630       +8     
  Lines         47518    47670     +152     
  Branches        414      421       +7     
============================================
+ Hits          24506    24762     +256     
+ Misses        22679    22571     -108     
- Partials        333      337       +4     
Files Coverage Δ
assets/admin/editor-wizard/patterns-list.js 50.00% <100.00%> (+23.33%) ⬆️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 735df53...6e4a751. Read the comment docs.

@merkushin merkushin added this to the 4.21.1 milestone Mar 8, 2024
@merkushin merkushin self-assigned this Mar 8, 2024
@merkushin merkushin requested a review from a team March 8, 2024 03:38
Copy link

github-actions bot commented Mar 8, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Mar 8, 2024

Test the previous changes of this PR with WordPress Playground.


/**
* Filters the warning message when no layouts are available.
*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donnapep Couldn't add @hook here. Is it expected?

CleanShot 2024-03-07 at 21 45 56@2x

@merkushin merkushin marked this pull request as ready for review March 8, 2024 03:48
Copy link

github-actions bot commented Mar 8, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @merkushin 👋

I just wanted to discuss an alternative approach in case we haven't already considered it, or in case we have already considered it but decided not to go with it for any reason, maybe we can document that too.

When there's no template available in the Template picker step, does it make better sense to skip that step altogether instead of showing a button to the user to go to that step, and then in the next step, showing a message that there is no pattern?

So when we are loading the steps array, we'll just check if patterns are available. And if patterns are not available, we'll just not load/filter out the patterns step. Because, in this case making the users go to the second step seems counterintuitive because they can't do anything there. Also if I was a new user creating my first Course, if I see a warning like that this early in my usage, I may not like it. Also, it shows a button "Start with default layout", but clicking on it doesn't use the default layout. Rather it just closes and goes to the empty editor.

LMKWYT?

cc: @donnapep for opinion on this

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, upon testing, I think checking for just available templates is not enough in our case here. Because for many other themes, it can hold true.

Astra:

Screenshot 2024-03-08 at 12 54 12 PM

Twenty Twenty Four:

Screenshot 2024-03-08 at 1 01 13 PM

Meraki:

Screenshot 2024-03-08 at 1 02 05 PM

In case of these theme-specific changes, I prefer testing it myself and also putting it in the testing instructions to test on other themes, both block and non-block ones, to make sure anything hasn't broken in other themes. Because they often end up doing that 😄

LMKWYT!

Copy link
Contributor

@Imran92 Imran92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think if we're adding a hook, we should add a Hooks label. Thanks!

@donnapep
Copy link
Collaborator

donnapep commented Mar 8, 2024

For the UI, I was hoping it could look similar to this (although horizontally and vertically-centered) instead of a notice:

Screenshot 2024-03-08 at 10 09 40 AM

@donnapep
Copy link
Collaborator

donnapep commented Mar 8, 2024

When there's no template available in the Template picker step, does it make better sense to skip that step altogether instead of showing a button to the user to go to that step, and then in the next step, showing a message that there is no pattern?

This would be preferred, but given this is an edge case (i.e. will only show for Divi users AFAIK), a simple solution seems better.

@merkushin
Copy link
Member Author

Oh, upon testing, I think checking for just available templates is not enough in our case here. Because for many other themes, it can hold true.

Thanks @Imran92! You're right. I have to check patterns instead.

I just wanted to discuss an alternative approach in case we haven't already considered it, or in case we have already considered it but decided not to go with it for any reason, maybe we can document that too.

Yes, we had those discussions here in GitHub and in Slack (p1709666714086119-slack-C02NWDZBL0H and p1709731277086819-slack-C02NWDZBL0H).

@merkushin
Copy link
Member Author

@donnapep Like that?

CleanShot 2024-03-08 at 22 44 40@2x

Fixed the condition and appearance here: 35585b0

Copy link

github-actions bot commented Mar 9, 2024

Test the previous changes of this PR with WordPress Playground.

@merkushin merkushin added the Hooks This change adds or modifies one or more hooks. label Mar 10, 2024
@donnapep
Copy link
Collaborator

@merkushin Yup! It would be nice if the font size was a bit larger (maybe to match the Start with default layout link?), but not a big deal.

@merkushin
Copy link
Member Author

Didn't see much difference after changing from 12px to 13px (size of "Start with default layout"), increased to 14px.
CleanShot 2024-03-11 at 12 34 27@2x

Copy link

Test the previous changes of this PR with WordPress Playground.

@merkushin merkushin merged commit 790e451 into trunk Mar 12, 2024
25 checks passed
@merkushin merkushin deleted the fix/editor-layout-with-divi branch March 12, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hooks This change adds or modifies one or more hooks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Course or Lesson Layouts on Divi
3 participants