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

pat contentbrowser: ui improvements for select button in toolbar #1411

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

1letter
Copy link
Contributor

@1letter 1letter commented Nov 18, 2024

  • use the title of content object as label of the select button
  • enable a bs tooltip with the path info of selected contentobject

the old behavior was "display the path", now "display the title" and show the path in a tooltip

@1letter
Copy link
Contributor Author

1letter commented Nov 18, 2024

@petschki @MrTango let me know what is your opinion for the requested solution?

@1letter 1letter requested review from petschki and MrTango November 18, 2024 16:11
@MrTango
Copy link
Contributor

MrTango commented Nov 19, 2024

Good idea. I was also wondering if it makes sense to just use the button for the word select and show the path/ title as text. It was not right away clear to me where i select a folder ;)

@1letter
Copy link
Contributor Author

1letter commented Nov 19, 2024

I have shown this solution to our power users/editors. They want a cleaner solution. ‘Less is more’ - just the “Select” button without text, but that would mean the button for the selection should be in the same column above, not in the next right column up. Perhaps is the solution of this PR overloaded? Are we to nerdy in our Plone world ? I convert this PR to a Draft PR.

@1letter 1letter marked this pull request as draft November 19, 2024 08:35
Copy link
Member

@thet thet left a comment

Choose a reason for hiding this comment

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

  1. I'd not use bootstrap in here but use the browser's title mechanism to show tooltips. See me suggestion in code.

  2. The commit message could be a bit more verbose about what your changes are about.

data-bs-toggle="tooltip"
data-bs-title="{level.displayPath}"
data-bs-placement="top"
data-bs-custom-class="contentbrowser-tooltip"
Copy link
Member

@thet thet Dec 3, 2024

Choose a reason for hiding this comment

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

wouldn't a title attribute be enough for showing a tooltip by browser mechanisms?
We could also augment that later using patternslib or something, if the browser tooltip isn't enough.

Background: I overheard that Twitter Bootstrap is in declining development state. And I get always a bit nervous when we add another dependency to Bootstrap.
Also, the browser tooltip mechanism via the title attribute is fine enough in most cases.

Copy link
Member

Choose a reason for hiding this comment

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

I second to not use Bootstrap here.

Comment on lines 437 to 440
data-bs-toggle="tooltip"
data-bs-title="{level.displayPath}"
data-bs-placement="top"
data-bs-custom-class="contentbrowser-tooltip"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data-bs-toggle="tooltip"
data-bs-title="{level.displayPath}"
data-bs-placement="top"
data-bs-custom-class="contentbrowser-tooltip"
title="{level.displayPath}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of simplifying it. I will change it.

@1letter 1letter marked this pull request as ready for review December 4, 2024 08:59
src/pat/contentbrowser/src/ContentBrowser.svelte Outdated Show resolved Hide resolved
@thet thet merged commit d2538d2 into master Dec 4, 2024
2 of 3 checks passed
@thet thet deleted the feat-pat-contentbrowser branch December 4, 2024 09:51
@MrTango
Copy link
Contributor

MrTango commented Dec 4, 2024

I'm wondering if it make sense at all, that we allow selection of parents. From the user point of view, it seems cleaner that you can only select from the current context, not above. The path element would be than only path elements and not buttons. From the UX, i could see, that we move the select button back inside the last column, where it is either an item (Page, Event, Image...) or another Folder. The Folder of course contains the list of items, but it could also have a select button above "select this container".

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

Successfully merging this pull request may close these issues.

4 participants