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

ContextMenu: add a css class to entries #10351

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

meven
Copy link
Contributor

@meven meven commented Oct 28, 2024

Change-Id: I71ce81efcbbb0c6a57d808d5f5d0e691567dfad5

  • Resolves: #
  • Target version: master

Summary

Context menu entries will now get a class corresponding to the uno
command name associated, in a snake-case format.

I.e InsertAnnotation => insert-annotation

This allows to change style individual context menu entry or even remove them.

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@meven meven requested a review from hfiguiere October 28, 2024 14:20
@meven
Copy link
Contributor Author

meven commented Oct 28, 2024

There is the L.installContextMenu that it doesn't cover.
I am not sure this is needed.

@meven
Copy link
Contributor Author

meven commented Oct 28, 2024

Hiding say "Insert Comment" context menu entry can be achieved with CSS:

li:has(.insert-comment) + li.context-menu-separator {
/* hide the next separator /*
  display: none;
}
li:has(.insert-comment)
{
/* hides the line that has the insert-comment action */
  display: none;
}

@hfiguiere
Copy link
Contributor

hfiguiere commented Oct 28, 2024

In Control.JSDialogBuilder.js there is _createIconURL which turn a UNO command name into and icon name. I think we should share the same logic here to be consistent.

@hfiguiere
Copy link
Contributor

the :has() pseudo selector might restrict which browsers this is compatible to. It's relatively recent in Firefox.

@meven
Copy link
Contributor Author

meven commented Oct 29, 2024

In Control.JSDialogBuilder.js there is _createIconURL which turn a UNO command name into and icon name. I think we should share the same logic here to be consistent.

The itemName here is not an uno command but some descriptive text from translation:
Uno command: uno:InsertAnnotation
itemName: _Insert_Comment
I wanted to match the end user point of view.

I could work with the uno-command, and use then the "insert-annotation" class instead. It would be consistent with the rest of the logic.

This function reuses same entries for different actions, I don't think it matches well my need:

			'insertannotation': 'shownote',
			'insertcomment': 'shownote',
			'objecttitledescription': 'shownote',
			'namegroup': 'shownote',

the :has() pseudo selector might restrict which browsers this is compatible to. It's relatively recent in Firefox.

Since 121 (Released 2023-12-19) and Firefox Mobile 121 (Released 2023-12-19). That's the first time I've been using it indeed.
I could try to put the class elsewhere in the dom to simplify the required CSS to hide the entry, but it would complicate more the change.
It is still up to the user to add the CSS.

@meven meven force-pushed the work/meven/context-menu-class branch from 087fc77 to c3d00e3 Compare November 1, 2024 08:59
@meven
Copy link
Contributor Author

meven commented Nov 1, 2024

Updated to use the more stable uno command as class, i.e InsertAnnotation => insert-annotation

@meven meven requested a review from eszkadev November 6, 2024 13:22
@hfiguiere
Copy link
Contributor

LGTM but I think this should have a ack from someone on the dev team.

@pedropintosilva
Copy link
Contributor

pedropintosilva commented Nov 7, 2024

multiple asserts failing https://cpci.cbg.collabora.co.uk:8080/job/github_online_master_debug_vs_co-24.04_cypress_desktop/2211/consoleFull but all of them seem unrelated (Calc: address input or calc paste). Pressing retry

@meven meven enabled auto-merge (rebase) November 15, 2024 11:15
@meven meven disabled auto-merge November 15, 2024 11:15
@meven
Copy link
Contributor Author

meven commented Nov 27, 2024

@pedropintosilva CI passed (finally), there was spurious build issue
Do you mind approving this ?

Copy link
Contributor

@pedropintosilva pedropintosilva left a comment

Choose a reason for hiding this comment

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

thanks this is a very good idea so, we can then if needed emphasize certain context menu entries etc!

The possible issue I see is that I don't follow why we need toSnakeCase or at least why we need the replacement of .... As far as I understand commandName never has .... The item.text on the other hand can have ellipsis but you are anyhow already commandName

@eszkadev
Copy link
Contributor

Could we put in the commit message some explanation? what exactly do we want to achieve here?

  • sample of "before/after result"
  • why that class is needed?
    This helps to understand -> review and later debug.

As I assume it is for icon?

@@ -309,9 +309,13 @@ L.Control.ContextMenu = L.Control.extend({
itemName = _UNO(item.command, docType, true);
}

var toSnakeCase = function (text) {
return text.replace(/[ _]/gi, '-').replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase().replace('...', '');
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we have more than 2 words? or single one?

can't we just do split and then join ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'/g' ensures it applies on all the string:

toSnakeCase("InserAnnotationTest")
=> "inser-annotation-test"

toSnakeCase("Insert test")
=> "insert-test"

toSnakeCase("Insert")
=> "insert"

@meven
Copy link
Contributor Author

meven commented Nov 28, 2024

The possible issue I see is that I don't follow why we need toSnakeCase or at least why we need the replacement of .... As far as I understand commandName never has .... The item.text on the other hand can have ellipsis but you are anyhow already commandName

The '...' is the remnant of previous design.
toSnakeCase is used to make uno command more css class names looking.
InsertAnnotation => insert-annotation
So css in the end looks better.

Context menu entries will now get a class corresponding to the uno
command name associated, in a snake-case format.

I.e InsertAnnotation => insert-annotation

This allows to change style individual context menu entry or even remove them.

Signed-off-by: Méven Car <[email protected]>
Change-Id: I71ce81efcbbb0c6a57d808d5f5d0e691567dfad5
@meven meven force-pushed the work/meven/context-menu-class branch from c3d00e3 to 25d6058 Compare November 28, 2024 17:18
@meven
Copy link
Contributor Author

meven commented Nov 28, 2024

Could we put in the commit message some explanation? what exactly do we want to achieve here?

* sample of "before/after result"

* why that class is needed?
  This helps to understand -> review and later debug.

As I assume it is for icon?

Could we put in the commit message some explanation? what exactly do we want to achieve here?

* sample of "before/after result"

* why that class is needed?
  This helps to understand -> review and later debug.

As I assume it is for icon?

I have added to the commit text:

Context menu entries will now get a class corresponding to the uno
command name associated, in a snake-case format.

I.e InsertAnnotation => insert-annotation

This allows to change style individual context menu entry or even remove them.

The current focus of this is to be able to hide context menu entries through CSS.

Note: We will need to have a filter for contextMenu entries following Hide_Command postMessage API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Review
Development

Successfully merging this pull request may close these issues.

4 participants