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

Implement slot for title in ListItem #3715

Closed
wants to merge 1 commit into from

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Feb 2, 2023

Signed-off-by: Maksim Sukharev antreesy.web@gmail.com

Problem

Right now there is no stable opportunity to manipulate 'Title' sub-component (see issues and PR below) - only through the access to DOM element via querySelector or using :deep selector for scoped styles.

Related:

Proposal

Add the ability to integrate a developer-controlled sub-component via Vue slot. Proposed solution should not affect current realisations, because required prop title is provided as a fallback for slot.

image

  • Examples for documentation is provided

⚠️ IMPORTANT

.list-item selector doesn't inherit box-sizing: border-box by default (If NcContent is not used). Fixed by this PR.

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy added enhancement New feature or request 3. to review Waiting for reviews feature: list-item Related to the list-item component labels Feb 2, 2023
@Antreesy Antreesy self-assigned this Feb 2, 2023
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Feb 2, 2023

I am not really fond of this solution. I think it's overkill to make this a slot, just for having the option to set the (native) title attribute. What's wrong with simply using <span class="line-one__title" :title="title">?

Edit: Although I have to admit that this is already a lot cleaner than the watch and DOM magic implemented in nextcloud/spreed#8605.

@Antreesy
Copy link
Contributor Author

Antreesy commented Feb 2, 2023

What's wrong with simply using <span class="line-one__title" :title="title">?

The trick is, we don't need this all the time. For example, in Talk it's only implemented for long names, which are covered with ellipsis and aren't fully visible. So it is better to have controlled tag, where you can apply attributes, logic, styles, e.t.c. locally.

I also want to note that, as it's an open-source library, we shouldn't mess with components working good before changes. So, attribute title could conflict with someone else local realisation.

@nickvergessen
Copy link
Contributor

For example, in Talk it's only implemented for long names, which are covered with ellipsis and aren't fully visible.

Which is a nice gimmic, but if that blocks a proper and performant solution I prefer to have the title just displayed all the time.

@raimund-schluessler
Copy link
Contributor

The trick is, we don't need this all the time. For example, in Talk it's only implemented for long names, which are covered with ellipsis and aren't fully visible. So it is better to have controlled tag, where you can apply attributes, logic, styles, e.t.c. locally.

If that really is a requirement, we can also introduce a new prop for it. Implementation wise, we could go the same way as in #3617, introduce a new name prop for what is now called title and make name fallback to the value of title if its not provided and use title as the native title value, to prevent a breaking change until the next major.

I also want to note that, as it's an open-source library, we shouldn't mess with components working good before changes. So, attribute title could conflict with someone else local realisation.

We cannot possibly anticipate all possible ways someone is using this library. What is not documented, is not supported in my opinion. We frequently introduce new features and props. If we couldn't do this because it might break someones undocumented and unintended use-case, we would be left with an unmaintained library without progress, because every little change could break something.

@Antreesy Antreesy marked this pull request as draft March 6, 2023 17:45
@susnux susnux added this to the 8.1.0 milestone Nov 8, 2023
@AndyScherzinger AndyScherzinger modified the milestones: 8.1.0, 8.1.1 Nov 14, 2023
@susnux susnux modified the milestones: 8.1.1, 8.2.0 Nov 16, 2023
@susnux susnux modified the milestones: 8.3.0, 8.4.0 Dec 1, 2023
@ShGKme ShGKme modified the milestones: 8.4.0, 8.5.0 Dec 27, 2023
@Pytal Pytal modified the milestones: 8.5.0, 8.6.0 Jan 23, 2024
@Antreesy
Copy link
Contributor Author

Superseded by nextcloud/spreed#11455

@Antreesy Antreesy closed this Jan 26, 2024
@Antreesy Antreesy removed this from the 8.6.0 milestone Jan 26, 2024
@ShGKme ShGKme mentioned this pull request Mar 11, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: list-item Related to the list-item component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants