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

move card to another board #1242

Merged
merged 6 commits into from Oct 14, 2019
Merged

move card to another board #1242

merged 6 commits into from Oct 14, 2019

Conversation

jakobroehrl
Copy link
Contributor

@juliushaertl
How can I retrieve the stacks from another board?
In the stacks store are only the stacks from the current board.
Thanks!

Signed-off-by: Jakob <jakob.roehrl@web.de>
@juliushaertl
Copy link
Member

Hm, so either we refactor the store to store all stacks and just return the ones related to the current board id or we just do a temporary api request to fetch the stacks from the foreign board in the modal component when trying to move. I think the second approach would be fine for now, since we do not really need the stack list for any other part of the app.

Signed-off-by: Jakob <jakob.roehrl@web.de>
@jakobroehrl
Copy link
Contributor Author

@juliushaertl
Tried to implement the API call.
Do you see a problem with that? I can't get the Stacks to the UI, but the data is there, proved with a console.log, thanks!

@jakobroehrl
Copy link
Contributor Author

@skjnldsv
Could you check this please?

stacksBySelectedBoard() {
if (this.selectedBoard === '') {
return []
}
let url = OC.generateUrl('/apps/deck/stacks/' + this.selectedBoard.id)
axios.get(url)
.then(
(response) => {
return Promise.resolve(response.data)
},
(err) => {
return Promise.reject(err)
}
)
.catch((err) => {
return Promise.reject(err)
}).then((result) => {
return result
})
},

I don't get the stacks on the UI, but a console.log in line 132 shows me the values.
Do you have an idea?

Thanks!

}

let url = OC.generateUrl('/apps/deck/stacks/' + this.selectedBoard.id)
axios.get(url)
Copy link
Member

Choose a reason for hiding this comment

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

Use try...catch instead of chaining .then and .catch :)
With async/await
This will help you figuring things out :)

Copy link
Member

@skjnldsv skjnldsv Sep 20, 2019

Choose a reason for hiding this comment

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

Also, you do not want to use a request in a computed.
Either load it once on beforeMount and store the results in a local data, or use the dedicated multiselect asynchronous method https://vue-multiselect.js.org/#sub-asynchronous-select

@juliushaertl
Copy link
Member

juliushaertl commented Oct 4, 2019

@jakobroehrl Would be cool if you could try assigning labels to represent the state of the pr, that way it is easier to see in the overview if they need some attention for reviewing. 😉

Edit: And the 1.0.0 milestone, so we don't miss one of the awesome features you do in the changelog when releasing.

Signed-off-by: Jakob <jakob.roehrl@web.de>
Signed-off-by: Jakob <jakob.roehrl@web.de>
@jakobroehrl
Copy link
Contributor Author

@juliushaertl Could you review please and merge if everything is fine?

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

It would be great if we could do some UI polishing to the modal before merging:

  • Button alignment (see other comments)
  • Add spacing to the multiselect boxes
  • Give the modal a more decent size, currently it looks way to big for the two options we have

Signed-off-by: Jakob <jakob.roehrl@web.de>
@@ -248,4 +296,13 @@ export default {
color: transparent;
}
}

.modal__content {
width: 25vw;
Copy link
Member

Choose a reason for hiding this comment

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

A proper max-width/max-height would be good for small screen sizes

.modal__content button{
float: right;

.multiselect {
Copy link
Member

Choose a reason for hiding this comment

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

.multiselect is not inside the button ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Jakob <jakob.roehrl@web.de>
@juliushaertl juliushaertl merged commit 5772a70 into vue Oct 14, 2019
@juliushaertl juliushaertl deleted the moveCardToBoard branch October 14, 2019 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants