-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
tickets views #1158
tickets views #1158
Conversation
Kanban view Compact view
sync last changes on days 27 and 28
Hey! This is some really nice work here, well done!! I've left a couple comments for you to look over. Generally speaking, I think we should exclude 'Closed' being a state on the board. I'm fine with resolved, but our stance with a ticket being closed is that you don't get to re-open it afterwards. Currently, the kanban bypasses the resolved -> closed flow and also lets you re-open tickets. Also, I'm thinking we should remove the ability to move columns around and rename Thoughts? :) |
I agree with you I did changes now to keep resolved rules that you mentioned, so Resolved will remain on columns |
"t probably also makes sense to hide the Kanban option when you are just viewing closed tickets (tickets.php?status=Closed) as you can't do anything with them." About this, makes all sense, will update to behave like that Done |
tickets_kanban.php
Outdated
|
||
<script src="/plugins/dragula/dragula.min.js"></script> | ||
|
||
<script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight nickpick, but can we move this into js/
and call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jhonny said somewhere that it was to go there on plugins... please tell me where to go and I can change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit is fine now that you've moved it to the plugins folder.
<script src="/plugins/dragula/dragula.min.js"></script>
--
Your custom JS (below) needs moving to its own .js file and then including like above.
<script>
$(document).ready(function() {
// Initialize Dragula for the Kanban board
var boardDrake = dragula([
document.querySelector('#kanban-board')
], {
moves: function(el, container, handle) {
return handle.classList.contains('panel-title');
}
});
[...]
And ideally same for your custom css further up the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @wrongecho script moved to /js
about css, where can we store the file? I guess we'll have to create a css folder, to put it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh, CSS used to have its own folder but we moved it all under vendor/plugins. It can go in with plugins/dragula/
, we don't need a dedicated folder just for it - just somewhere to keep it organised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wrongecho @johnnyq This css is related to the page tickets_kanban.php and not dragula itself. Still place it under dragula folder?
Hey ssteeltm, Nice changes, thank you! I'm really excited to get this reeled in. We still need to make sure all output is sanitized and unfortunately SonarCloud doesn't catch everything. You can use htmlspecialchars (or our own nullable_htmlentities) to prevent XSS injection from asset names like this: -- Is there a reason the Kanban doesn't show the ticket number and includes a blank circle at the top? I thought it was for client tags but apparently not? --
Potentially? We'll likely still want to add the ability to manage ticket_status_order via the admin settings and use it as an order by elsewhere.. Let's see what @johnny says. |
Hey! glad ur enjoying this like me! I'll do the updates |
I'm listing here the TODO:
|
Probably worth mentioning that our general principle is keeping things simple with good/opinionated defaults. In general we try and avoid making everything confusing and ultra-customisable. Therefore, I'd say we either include the ability to order tickets & move columns, or we don't. Not everything needs to be customisable. I mentioned somewhere on the forum it'd be nice to add the default ticket view in settings. Defaults will be the best place for that but it might be best we add that separately after this is merged in the interests of getting this pulled in and tested as part of 25.02. |
I do like the default ticket view in the the ticket setting section. I think we should eventually move away from the defaults settings tab and move the entities in their respectable places such as invoice, ticket, quote etc settings. Its more logical. |
|
@johnnyq @wrongecho guess its done for first merge. Please review last changes |
@ssteeltm looks good, give me some time to review everything once more then we'll pull it in |
Its looking good @ssteeltm im gonna reel it on in and we can make chnages if necessary during the Feb dev phase. |
Kanban view
Compact view