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

Adding an option to hide themes #13723

Merged
merged 7 commits into from May 13, 2024
Merged

Adding an option to hide themes #13723

merged 7 commits into from May 13, 2024

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented May 6, 2024

Q A
Bug fix? (use the a.b branch)
New feature/enhancement? (use the a.x branch)
Deprecations?
BC breaks? (use the c.x branch)
Automated tests included?
Related user documentation PR URL mautic/user-documentation#303
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed /

Description:

Give user option to hide/unhide 'default' themes in the themes list page
Screenshot 2024-05-06 at 12 15 31
Screenshot 2024-05-06 at 12 15 40
Screenshot 2024-05-06 at 12 15 55

Do no show these hidden themes in the Email/Page theme list
Update existing themes using migration

Steps to test this PR:

MAUT-10202:

  1. Go to themes, check if 'Hide' option is visible for the default themes only
  2. On click of 'Hide' option in the dropdown, confirmation popup appears.
  3. On confirmation, theme is moved to the bottom of the theme list and is disabled (shown in grey font) and theme is hidden from email/page theme list and also in the
  4. Hidden themes will have 'Unhide' menu option, on clicking confirmation popup opens, on confirming theme will unhide and will be visible in the previous order and also visible in the email/page theme list
  5. We have added a migration to rename deleted.txt with hidden-themes.txt and remove custom theme names from the file which are present in the instance's public_html/themes directory.

MAUT-10140:

  1. There is no option to delete default themes anymore (this is now changed to Hide/Unhide)
  2. Custom themes can be deleted, these won't be added to deleted.txt (deleted.txt has been renamed to hidden-themes.txt)
  3. Saving custom themes with already existing names will generate error for e.g. Template name abc already exists.
  4. Since, saving a theme is prefixed with 'beefree-' by default, if we try to save a befree theme without the 'beefree-' keyword, will generate error for e.g. If we try to save them with name fall-in-love-with-this-seasons-collection will generate - beefree-fall-in-love-with-this-seasons-collection is the default theme and therefore cannot be overwritten.

Copy link

codecov bot commented May 6, 2024

Codecov Report

Attention: Patch coverage is 78.48101% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 61.53%. Comparing base (4f420ab) to head (8872a9b).
Report is 11 commits behind head on 5.x.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13723      +/-   ##
============================================
+ Coverage     61.50%   61.53%   +0.02%     
- Complexity    34068    34098      +30     
============================================
  Files          2241     2241              
  Lines        101852   101930      +78     
============================================
+ Hits          62647    62719      +72     
- Misses        39205    39211       +6     
Files Coverage Δ
app/bundles/CoreBundle/Helper/ThemeHelper.php 76.62% <88.46%> (+5.67%) ⬆️
.../bundles/CoreBundle/Controller/ThemeController.php 37.12% <59.25%> (+5.69%) ⬆️

... and 3 files with indirect coverage changes

@escopecz escopecz added ready-to-test PR's that are ready to test enhancement Any improvement to an existing feature or functionality themes Anything related to themes unforking Used for PRs in the Acquia's unforking initiative labels May 6, 2024
@RCheesley RCheesley requested review from a team, andersonjeccel and ricfreire May 7, 2024 13:44
@RCheesley
Copy link
Sponsor Member

A couple of questions:

  1. You mention default themes, can users also apply this to their own custom themes?
  2. Could we update to use the new icon pack (ri-eye-off-line was suggested in this comment)
  3. Can we have some user/developer docs please 😺

@escopecz
Copy link
Sponsor Member Author

escopecz commented May 7, 2024

Good questions

  1. It doesn't make sense for custom themes as those can be easily removed. The default ones cannot and that's why the show/hide feature is needed.
  2. I updated it in 3a20c42 and I also had to change the icon from preview otherwise the eye icon would be used for preview as well as for unhide options. This is how it looks like now:
    Screenshot 2024-05-07 at 16 11 01
    Screenshot 2024-05-07 at 16 10 55
  3. There is nothing needed for developer docs. It's a new column in the database. Nothing developers can set in the theme itself. Here is the user doc change: Hide/Unhide themes documented user-documentation#303

@RCheesley RCheesley added the T1 Low difficulty to fix (issue) or test (PR) label May 7, 2024
Copy link
Contributor

@andersonjeccel andersonjeccel left a comment

Choose a reason for hiding this comment

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

From the UX perspective, that's a nice addition!

Copy link
Contributor

@shinde-rahul shinde-rahul left a comment

Choose a reason for hiding this comment

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

LGTM!!

Thanks @escopecz for having this feature in core.

Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

On add/edit langing page I got error:

 500 Internal Server Error - Key "visibility" for array with keys "name, key, config, dir, themesLocalDir" does not exist. 
 
 /html/app/bundles/CoreBundle/Resources/views/Helper/theme_select.html.twig:37 at

Other parts seems work properly.

…g it to the less file and running `grunt compile-less` to generated it to css properly
@escopecz
Copy link
Sponsor Member Author

escopecz commented May 9, 2024

@kuzmany I couldn't reproduce but I fixed it based on the error message you provided.

I also noticed that the CSS class was added to libraries.css and not to any less file. So I fixed that too. It's not my code. I'm just pushing it to the community repo, so I'm not that familiar with it.

@escopecz escopecz requested a review from kuzmany May 9, 2024 15:16
Copy link

@Esthertests Esthertests left a comment

Choose a reason for hiding this comment

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

great changes
image
image
image
Tested the email page and checked that an hidden template is not shown and once unhidden it is shown,
image

@Esthertests
Copy link

@escopecz can we add a notification panel shown when user unhides a theme

Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Works properly 👍

@kuzmany kuzmany added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed ready-to-test PR's that are ready to test labels May 11, 2024
@escopecz escopecz added this to the 5.1.0 milestone May 13, 2024
@escopecz escopecz merged commit ca2bf45 into mautic:5.x May 13, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T1 Low difficulty to fix (issue) or test (PR) themes Anything related to themes unforking Used for PRs in the Acquia's unforking initiative ux-review-passed
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants