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 attachments to desc #1740

Merged
merged 3 commits into from
Apr 27, 2020
Merged

add attachments to desc #1740

merged 3 commits into from
Apr 27, 2020

Conversation

jakobroehrl
Copy link
Contributor

@jakobroehrl jakobroehrl commented Apr 21, 2020 •
edited by juliusknorr
Loading

Signed-off-by: Jakob Röhrl jakob.roehrl@web.de

TODO

  • CSS
  • "this.markdownEditor is undefined" error on adding the attachment

juliusknorr
juliusknorr reviewed Apr 21, 2020
View reviewed changes
src/components/card/CardSidebar.vue
@@ -127,6 +127,11 @@
href="https://deck.readthedocs.io/en/latest/Markdown/"
target="_blank"
class="icon icon-info" />
<Actions v-if="canEdit">
<ActionButton v-if="descriptionEditing" icon="icon-attach" @click="showAttachmentModal()">
Copy link
Member

Choose a reason for hiding this comment

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

Something is wrong with the sizing of that icon make sure that the background iamge is only 16x16px.

@juliusknorr juliusknorr added this to the 🚀 Next major 1.0.0 milestone Apr 21, 2020
@juliusknorr juliusknorr linked an issue Apr 21, 2020 that may be closed by this pull request
attachment in card description fails in Safari #907
Closed
@jakobroehrl jakobroehrl self-assigned this Apr 22, 2020
@jakobroehrl jakobroehrl force-pushed the enh/addAttachments branch 2 times, most recently from 4f15abc to 33fa460 Compare April 23, 2020 11:10
@jakobroehrl jakobroehrl requested a review from juliusknorr April 24, 2020 10:22
juliusknorr
juliusknorr reviewed Apr 24, 2020
View reviewed changes
src/components/card/AttachmentList.vue Outdated
type: Number,
required: true,
},
editor: {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I'm not a big fan of that appraoch as this bundles the AttachmentList with the implementation of the editor. Having an event would be much nicer here

juliusknorr
juliusknorr reviewed Apr 24, 2020
View reviewed changes
src/components/card/CardSidebarTabAttachments.vue
float: right;
margin: 40px 3px 3px 0;
}

.attachment-list {
Copy link
Member

Choose a reason for hiding this comment

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

This can also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? There is still an attachment-list entry:
https://github.com/nextcloud/deck/blob/a7473e9ec9bca51c435a36bfd4255e17e5291f22/src/components/card/CardSidebarTabAttachments.vue#L33

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought that would already use the new AttachmentList component. Then OK, we can also migrate that later on, maybe you can add a TODO comment for that here.

juliusknorr
juliusknorr reviewed Apr 27, 2020
View reviewed changes
package-lock.json
@@ -13068,7 +13068,7 @@
},
"os-homedir": {
"version": "1.0.2",
"resolved": "https://registry.npmjs.org/os-homedir/-/os-homedir-1.0.2.tgz",
Copy link
Member

Choose a reason for hiding this comment

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

Please don't commit such changes 😉 I assume you ran npm install instead of npm ci.

jakobroehrl and others added 3 commits April 27, 2020 16:13
@jakobroehrl @juliusknorr
add attachments to desc
9b8bb8f 
Signed-off-by: Jakob Röhrl <jakob.roehrl@web.de>
@jakobroehrl @juliusknorr
in component outsourced
0659038 
Signed-off-by: Jakob Röhrl <jakob.roehrl@web.de>
@juliusknorr
Make sure event is properly emitted
7c15a17 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr mentioned this pull request Apr 27, 2020
Closed
2 tasks
@juliusknorr juliusknorr merged commit b5d0c62 into master Apr 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the enh/addAttachments branch April 27, 2020 14:31
@juliusknorr juliusknorr mentioned this pull request Sep 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

Assignees

@jakobroehrl jakobroehrl

Labels
4. to release enhancement high
Projects
None yet
Milestone
🚀 Next major 1.0.0
Development

Successfully merging this pull request may close these issues.

Button to add attachment to the description is missing attachment in card description fails in Safari
2 participants
@jakobroehrl @juliusknorr