THREESCALE-11731: Fix cms button styles #4041

mayorova · 2025-03-13T09:53:19Z

What this PR does / why we need it:

The styles of the CMS buttons were all mixed up.

Before and after:
NOTE: on the screencast the red buttons are not yet updated

before.mp4

after.mp4

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-11731

Verification steps

Special notes for your reviewer:

josemigallas · 2025-03-13T16:23:10Z

app/assets/stylesheets/provider/admin/cms/_templates.scss

@@ -101,6 +101,7 @@

 #cms-new-content-button {
  position: absolute;
+  margin: 16px;


Suggested change

margin: 16px;

margin: var(--pf-global--gutter--md);

josemigallas · 2025-03-13T16:31:40Z

app/javascript/packs/pf_form.scss

@@ -1,5 +1,6 @@
 @import '~@patternfly/patternfly/components/Button/button.css';
 @import '~@patternfly/patternfly/components/Check/check.css';
+@import '~@patternfly/patternfly/components/Dropdown/dropdown.css';


This will include dropdown.css into all existing forms, but Dropdown is not even a form element (contrary to FormSelect). I'd suggest creating a new pack cms.scss that will be loaded automatically here:

porta/app/views/layouts/_cms.html.erb

Line 4 in 3eaf100

<%= javascript_packs_with_chunks_tag 'codemirror', 'cms' %>

This has a very low impact in performance, but still.

josemigallas · 2025-03-13T16:34:24Z

app/javascript/packs/cms.ts

+function setUpPreviewButton () {
+  $(document).on('click', '#cms-preview-button button.pf-c-dropdown__toggle-button:not(.dropdown-toggle)', (event) => {
+    const url = event.target.dataset.url
+    if (url) {


Is it ever going to be undefined?

Hmm... in theory, data-url should always be present, but I don't know... if for some reason (a developer error etc.) it's not there, I think it's not a bad idea to check.

josemigallas · 2025-03-13T16:34:47Z

app/javascript/packs/cms.ts

+  $(document).on('click', '#cms-preview-button button.pf-c-dropdown__toggle-button:not(.dropdown-toggle)', (event) => {
+    const url = event.target.dataset.url
+    if (url) {
+      window.open(url)


Open in a new tab, maybe?

Suggested change

window.open(url)

window.open(url, 'previewTab')

hmm, what's previewTab ? 🤔 I can see _self, _blank and others here: https://developer.mozilla.org/en-US/docs/Web/API/Window/open#target but not previewTab.

The default is _blank which already opens in a new tab.

josemigallas · 2025-03-13T16:35:53Z

app/lib/cms/semantic_form_builder.rb

+    def disabled_delete_button
+      template.link_to('Delete', '',
+                       class: 'delete pf-c-button pf-m-danger pf-m-disabled',
+                       title: "You can not delete this #{@object.class.model_name.human.downcase} because it's being used")


Suggested change

title: "You can not delete this #{@object.class.model_name.human.downcase} because it's being used")

title: "You cannot delete this #{@object.class.model_name.human.downcase} because it's being used")

josemigallas · 2025-03-13T16:42:46Z

app/views/provider/admin/cms/_new_button.html.erb

Should we remove this partial and render cms/_dropdown directly?

Yeah, given the fact that we already have this cms_new_button_dropdown helper method, we can as well just move this content there.

josemigallas · 2025-03-13T16:49:50Z

app/javascript/packs/cms.ts

- * The CMS features 3 dropdown buttons: New button, preview button and save button. Html is rendered
- * by CMS::DropdownHelper and its functionality is given by this method everytime a new template is
+ * The CMS features 4 dropdown buttons: New button, Preview, Publish and Save. Html is rendered
+ * by "/provider/admin/cms/dropdown" partial and its functionality is given by this method everytime a new template is
 * selected (cms-sidebar:update event).
 *
 * The event handler has to be defined with jQuery because at the time this method is called, the
 * CMS content is not yet rendered and document.querySelector would find nothing.
 */
 function setUpDropdownButtonOpen () {
  $(document).on('click', '.dropdown-toggle', (event) => {


This should be a great opportunity to remove jquery from here. It had to be like that because the dropdown was rendered dynamically and jQuery sets events listener globally.

Now the dropdown renders with the rest of the page and a listener can be set after dom is ready (DOMContentLoaded).

We can also drop .dropdown-toggle and use PF's classes. There is a similar snippet already https://github.com/3scale/porta/blob/fix-cms-button-styles/app/javascript/packs/authentication_providers.ts

OK, I see the listeners situation. In fact, I didn't fully realize that that Save dropdown was built dynamically 😅 But yes, after I changed it to dropdown, buildSaveDropdownButton seemed useless for sure.

The problem with this dropdown button is that the actual button and the toggle (with the arrow) have the same classes - pf-c-dropdown__toggle-button, see https://pf4.patternfly.org/components/dropdown#split-button-primary-action
That's why I had to use a custom class. We could of course search for a ".pf-c-dropdown__toggle-button that has a nested i.fa-caret-down", but I think it's more confusing and complex.

mayorova · 2025-03-13T23:48:23Z

app/assets/stylesheets/provider/_buttons.scss

@@ -74,13 +74,6 @@ a.next {
  @include internal-potato-button(white, $delete-color, $delete-color );
 }

-
-.formtastic .actions {


To be honest, I am not 100% sure this does not have any negative effect on any existing forms... But I think there are very few forms left that would use it.

mayorova · 2025-03-14T14:57:48Z

app/javascript/packs/cms.ts

+/**
+ * Set up CMS Preview button to act as a link
+ */
+function setUpPreviewButton () {


The purpose of this function is to open a URL stored in the data-url attribute of the <button> element named "Preview". In the previous implementation it was an <a> element, so it was not needed. However, in PF4 dropdown it should be a button.

I could have just added an onclick handler to the button (like I did for the "New X" button), but in this case the URL is not static, and can get updated if the path of the template is changed, see app/views/provider/admin/cms/templates/update.js.erb.

mayorova · 2025-03-14T14:59:53Z

app/views/provider/admin/cms/templates/update.js.erb

  $('#cms-preview-button a').each(function () {
    if ($(this).data('preview') === 'draft') {
      $(this).attr('href', '<%= cms_draft_url(@page) %>');
    }
  });
+  const previewButton = $('#cms-preview-button button.pf-c-dropdown__toggle-button:not(.dropdown-toggle)')


The first piece that was already there (for #cms-preview-button a) is for the entries in the expanded dropdown, and the new piece is for the toggle button, which holds the URL in the data-url attribute.

I have no idea why the published link is not updated, I think it might be a bug, but I haven't touched it.

mayorova · 2025-03-14T16:43:50Z

app/javascript/packs/cms.ts

+      toggle.addEventListener('click', () => {
+        closeAllDropdowns(toggle)
+
+        /* eslint-disable @typescript-eslint/non-nullable-type-assertion-style */


@josemigallas Do we need to have this rule enabled? 🤔 I don't see why it might be needed.
It kind of suggest (from what I can understand) to use ! instead of type cast, but then I am having the following rule violation: https://typescript-eslint.io/rules/non-nullable-type-assertion-style/

mayorova added 5 commits March 12, 2025 15:33

PF4 styles for CMS preview-publish-save buttons

1995a6e

Add margins for the CMS New Page button dropdown

921e4f5

Use PF4 style for CMS New dropdown and remove leftovers

4a29a3c

Refactor dropdown toggle

b26933c

Update CMS dropdown cucumber steps

75ff825

mayorova changed the title ~~Fix cms button styles~~ THREESCALE-11731: Fix cms button styles Mar 13, 2025

Update styles of the Delete and Revert buttons

3eaf100

mayorova marked this pull request as ready for review March 13, 2025 13:43

josemigallas reviewed Mar 13, 2025

View reviewed changes

mayorova commented Mar 13, 2025

View reviewed changes

mayorova commented Mar 14, 2025

View reviewed changes

mayorova added 3 commits March 14, 2025 17:10

Use a variable for CMS New button margins

9b8f57c

Remove the _new_button partial and render the dropdown in the helper

da205ca

Refactor CMS JS to reduce jQuery usage

e4f8c73

mayorova force-pushed the fix-cms-button-styles branch from a45297a to e4f8c73 Compare March 14, 2025 16:40

mayorova commented Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-11731: Fix cms button styles #4041

THREESCALE-11731: Fix cms button styles #4041

mayorova commented Mar 13, 2025 •

edited

Loading

josemigallas Mar 13, 2025

josemigallas Mar 13, 2025

josemigallas Mar 13, 2025

mayorova Mar 14, 2025

josemigallas Mar 13, 2025

mayorova Mar 14, 2025

josemigallas Mar 13, 2025

josemigallas Mar 13, 2025

mayorova Mar 13, 2025

josemigallas Mar 13, 2025

mayorova Mar 14, 2025

mayorova Mar 13, 2025

mayorova Mar 14, 2025

mayorova Mar 14, 2025

mayorova Mar 14, 2025

	title: "You can not delete this #{@object.class.model_name.human.downcase} because it's being used")
	title: "You cannot delete this #{@object.class.model_name.human.downcase} because it's being used")

THREESCALE-11731: Fix cms button styles #4041

Are you sure you want to change the base?

THREESCALE-11731: Fix cms button styles #4041

Conversation

mayorova commented Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayorova commented Mar 13, 2025 •

edited

Loading