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

feat(NcNoteCard): provide a slot for inserting a custom icon instead of default #4894

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

Antreesy
Copy link
Contributor

☑️ Resolves

  • Allows to replace default icon with a custom one
  • If no custom icon is provided, default is used as a fallback for a slot
  • Reduce efforts for implementing a workarounds and extra wrappers, if custom icon is needed

🖼️ Screenshots

🏡 After
image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@Antreesy Antreesy added enhancement New feature or request 3. to review Waiting for reviews feature: note-card Related to the note-card component labels Nov 30, 2023
@Antreesy Antreesy added this to the 8.2.1 milestone Nov 30, 2023
@Antreesy Antreesy self-assigned this Nov 30, 2023
@susnux
Copy link
Contributor

susnux commented Nov 30, 2023

What is the usecase for this?

@Antreesy
Copy link
Contributor Author

What is the usecase for this?

As always, a Talk repo. Styles of NoteCard component are fitting for Out-of-Office disclaimer to not reinvent this component from scratch, but I need to put a user's avatar instead of blue exclamation icon

image

@ShGKme
Copy link
Contributor

ShGKme commented Nov 30, 2023

What is the usecase for this?

As always, a Talk repo. Styles of NoteCard component are fitting for Out-of-Office disclaimer to not reinvent this component from scratch, but I need to put a user's avatar instead of blue exclamation icon

Could avatar be the content of the note instead of an icon?

IMO, it is a very edge case for this component, and avatar should not be used as an icon.

@Antreesy
Copy link
Contributor Author

Antreesy commented Nov 30, 2023

Could avatar be the content of the note instead of an icon?

Yes, but It is still requires to hide default icon from the DOM, and reinvent a bicycle to wrap a new icon and a content

it is a very edge case for this component

Agree, but it's wider than just an avatar, and more a nice-to-have option (which we have at NcAction... components, for example)

@susnux
Copy link
Contributor

susnux commented Nov 30, 2023

Agree, but it's wider than just an avatar, and more a nice-to-have option (which we have at NcAction... components, for example)

Yes but for the avatar this would be the wrong place and should not be placed there.

And I am no designer but I think that this might make thing more inconsistent design wise (for all other cases). But as said I am not a designer.

@susnux susnux modified the milestones: 8.2.1, 8.3.0, 8.4.0 Nov 30, 2023
@ShGKme ShGKme modified the milestones: 8.4.0, 8.5.0 Dec 27, 2023
@susnux
Copy link
Contributor

susnux commented Jan 23, 2024

image

@nextcloud-libraries/designers is this something we want?

If yes then lets go with it :)

@Pytal Pytal modified the milestones: 8.5.0, 8.6.0 Jan 23, 2024
@szaimen
Copy link
Contributor

szaimen commented Jan 24, 2024

image

@nextcloud-libraries/designers is this something we want?

If yes then lets go with it :)

what exaclty? (the screenshot is missing)

@ShGKme
Copy link
Contributor

ShGKme commented Jan 24, 2024

what exaclty? (the screenshot is missing)

Fixed

@susnux
Copy link
Contributor

susnux commented Jan 24, 2024

what exaclty?

Having a custom icon in the note card, or more specific should we omit the "info" icon if there is an avatar.

@szaimen
Copy link
Contributor

szaimen commented Jan 24, 2024

Having a custom icon in the note card, or more specific should we omit the "info" icon if there is an avatar.

Yes, makes sense imho. But I would design that thing specifically like this:
image

@Antreesy
Copy link
Contributor Author

Antreesy commented Jan 24, 2024

Yes, makes sense imho. But I would design that thing specifically like this

What do you mean by that? Either a default icon or an avatar if provided? Wouldn't it be overcomplicated for that component?

@szaimen
Copy link
Contributor

szaimen commented Jan 24, 2024

I just wanted to point out that the design for this specific feature could be improved. It is up to you to decide if such a design would make sense as separate component or to overwrite the design of the component in talk then...

@Antreesy
Copy link
Contributor Author

I don't see it as a wide use-case to make it a separate component. That's why I'd just put a slot as proposed by this PR and do the rest in the app itself.
Feature design is also up to Talk repo to discuss, so we can proceed with it there

@Antreesy Antreesy requested a review from susnux January 29, 2024 09:14
@Antreesy Antreesy requested a review from szaimen January 29, 2024 09:14
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

@Antreesy Antreesy merged commit a0c316f into master Jan 29, 2024
15 checks passed
@Antreesy Antreesy deleted the feat/noid/note-card-icon-slot branch January 29, 2024 16:55
@Antreesy
Copy link
Contributor Author

/backport to next

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: note-card Related to the note-card component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants