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.

Sign up for GitHub

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

Jump to bottom

Add info NoteCard #4063

Merged
merged 1 commit into from May 19, 2023
Merged

Add info NoteCard #4063

merged 1 commit into from May 19, 2023

Conversation

moan0s
Copy link
Contributor

@moan0s moan0s commented May 8, 2023

This is a proposal for #4020 and linked to nextcloud/server#37953

I am net in any way experienced with this, so feel free to point out any errors. I am also not set on the colors, it's just what felt kinda right (but again, I am not a frontend dev)

szaimen
szaimen approved these changes May 8, 2023
View reviewed changes
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.

Thanks for your PR! I think it should work with the changes below.

cc @jancborchardt @nimishavijay

src/components/NcNoteCard/NcNoteCard.vue Show resolved Hide resolved
src/components/NcNoteCard/NcNoteCard.vue Show resolved Hide resolved
styleguide/assets/default.css Outdated Show resolved Hide resolved
@szaimen szaimen added the 3. to review Waiting for reviews label May 8, 2023
szaimen
szaimen requested changes May 8, 2023
View reviewed changes
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.

Clicked the wrong button, see the feedback above

@szaimen szaimen mentioned this pull request May 8, 2023
Merged
5 tasks
@nimishavijay
Copy link

Nice! Thank you for the contribution :) Could you please share a screenshot for easy design review by anyone else also? :)

szaimen
szaimen approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment •
edited

Choose a reason for hiding this comment

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

Works and looks good. Thanks a lot! :)

I took the liberty to add it to the examples.

@moan0s for us being able to merge this, can you please fix DCO? See https://github.com/nextcloud/nextcloud-vue/pull/4063/checks?check_run_id=13318409670 for details

@moan0s
Copy link
Contributor Author

moan0s commented May 8, 2023

Oh yes, sorry I forgot that and did not check the CI. Should be fixed now

@szaimen szaimen requested review from a team, susnux, Fenn-CS, Pytal and jancborchardt and removed request for a team May 8, 2023 16:29
@mejo- mejo- mentioned this pull request May 9, 2023
Closed
@susnux susnux mentioned this pull request May 11, 2023
Merged
1 task
@szaimen szaimen reopened this May 16, 2023
@szaimen
Copy link
Contributor

szaimen commented May 16, 2023 •
edited

@szaimen szaimen changed the title Add info icon Add info NoteCard May 19, 2023
@moan0s @szaimen @raimund-schluessler
Add info NoteCard
ec87423 
Signed-off-by: Julian-Samuel Gebühr <julian-samuel@gebuehr.net>
Co-Authored-By: Simon L. <szaimen@e.mail.de>
Co-Authored-By: Raimund Schlüßler <raimund.schluessler+github@mailbox.org>
szaimen
szaimen approved these changes May 19, 2023
View reviewed changes
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.

Good to go from my side:
image

@szaimen szaimen added the enhancement New feature or request label May 19, 2023
@szaimen szaimen enabled auto-merge May 19, 2023 09:01
mejo-
mejo- approved these changes May 19, 2023
View reviewed changes
Copy link
Contributor

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Code looks good. Thanks a lot for tackling this @moan0s.

@szaimen szaimen merged commit 4142020 into nextcloud-libraries:master May 19, 2023
16 checks passed
@moan0s
Copy link
Contributor Author

moan0s commented May 19, 2023

Thanks for doing all the real work, I very much appreciate it!

@max-nextcloud
Copy link
Contributor

/backport to stable7

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jun 16, 2023
Closed
@max-nextcloud
Copy link
Contributor

Not backported due to the dependency on Nextcloud 27.

@susnux
Copy link
Contributor

susnux commented Jun 16, 2023

@max-nextcloud we can add a fallback on the css vars on stable7 so it will work on all versions. (If there is a need for this)

@max-nextcloud
Copy link
Contributor

@susnux I was thinking the same but then decided to add the fallback inside text: nextcloud/text@c564511 . Pretty happy with that as it will also be easy to backport whereas using the NoteCard component would have required more changes.

@susnux susnux mentioned this pull request Jun 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejo- mejo- mejo- approved these changes

@szaimen szaimen szaimen approved these changes

@susnux susnux Awaiting requested review from susnux susnux was automatically assigned from nextcloud/server-frontend

@Fenn-CS Fenn-CS Awaiting requested review from Fenn-CS Fenn-CS was automatically assigned from nextcloud/server-frontend

@Pytal Pytal Awaiting requested review from Pytal Pytal was automatically assigned from nextcloud/server-frontend

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@moan0s @nimishavijay @szaimen @max-nextcloud @susnux @jancborchardt @raimund-schluessler @mejo-