-
Notifications
You must be signed in to change notification settings - Fork 277
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: #2906 add card ID badge #3757
Conversation
@juliushaertl can this be marked as to review please? |
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.
I have no problem with displaying the card id on the card directly - but honestly, it just looks completely odd. @nextcloud/designers do you see any way to add this without completely changing the card layout (which is currently aligned with the Deck Android app)?
I'd say we can add it to the list of card badges on the bottom left, but would also appreciate feedback from @nimishavijay and @jancborchardt about that and how we should handle this in terms of the extra setting for showing/hiding the number. Note that the id is unique across users/boards so on larger instances this might quickly become a number with >10 digits |
@juliushaertl that is why I placed it above the title, already tested with up to 12 digits and it works without impact. |
How does it help a user to quickly see the 12 digit id of a card? That's not really a number you memorize for refering to a card in a conversation, no? |
Good point @raimund-schluessler, which @stefan-niedermann also related to in the issue of this PR last March. However having a possible max of 19 digits (max of BIGINT which is I assume used as DB field type) in the first place is more a design flaw than an issue of this PR itself. Each card should imho have a proper If we look at other popular tools:
So their card numbering is depending on the board and thus gets a global proper However, I did think about that issue with bigger numbers and also users who don't want this feature in the first place as it might not fit into their workflow. And this is precisely why I added a setting to enable it manually. So in everyones best interest we could define that it is disabled by default, so it doesn't impact anyone's current setup and those who need it can enable it. |
Super nice work! :)
This seems like a good idea! It could go in the beginning of the badges, something like
This kind of ID seems really technical. Deck is usually compared with Trello, so a simpler ID like that seems more friendly. Currently all cards have unique IDs across all boards. How about cards within one board have unique IDs? For eg.
Instead we could do
This wouldn't be the same as the ID in the URL, but more like a reference number for each card. It also makes sense that when you create a new board and add a card it shows
Agreed, there is an addon in Trello for this and it seems quite popular too, so it is okay to have a setting for this since it's a feature that would be useful to power users but maybe not so much to just everyday users. @jancborchardt any thoughts about this? |
4 days later - How are we going to proceed here? |
@jancborchardt, any updates from your end? |
Hi, sorry @oneWaveAdrian 👍’d @nimishavijay’s proposal but let me expand:
However, there’s the big confusion about the usefulness of the ID as @juliushaertl mentioned:
So this is quite strange, and isn’t that a possible privacy problem too? I would agree with @nimishavijay that if we do show IDs in the UI, they should start at |
@jancborchardt I get the idea behind this: you can move a card between boards and external links to the card still work. So the card can be found even if, let's say it moves from the "User feedback" board to the "In development" board. Hence my suggestion to add a local ID (as @nimishavijay suggested starting with @nimishavijay you mentioned placing it at the beginning of the badges. What happens if an ID on a boarddoes get up to let's say |
Well, in the worst case when the entire bottom row is filled out there would be the card ID, description badge, attachment badge, assignees, and 3dot menu. If there is no space to show all these, we could start hiding the description and attachment badges, and if the ID is so big that there's really no space even after that it should be okay if we add a row below the ID and move the badges, assignees and 3dot menu there (although if the IDs are that large it would not be of much use anyway like @raimund-schluessler mentioned) |
Thanks for the design feedback @nimishavijay . So, @juliushaertl, @stefan-niedermann, @raimund-schluessler what are the next steps here in your opinion? Can the card Model be altered to accommodate a local ID? This is more than just a frontend change, but rather a change in card database architecture, therefore I would like some discourse/agreements before starting to alter things and then the change being rejected. |
one Month later - what's the decision on this @juliushaertl @jancborchardt @nimishavijay @stefan-niedermann? |
This comment was marked as off-topic.
This comment was marked as off-topic.
As another PR has popped up trying to provide the same feature, this seems to be something a lot of users need/want, can we move on with this topic? Even if you decide to close it, leaving it hanging mid-air forever is not an option to work with your community @juliushaertl @nimishavijay @jancborchardt |
Thinking about this a bit more I'd say even the global unique id is fine, as we also expose it for files with internal links. This might especially be useful if we introduce automatic references as GitHub has them with nextcloud/server#31667, so I'd be fine to merge this with the id badge being moved to the bottom row. That being said, from a user perspective I could see that a local id might still be useful, but I'd consider that a follow up feature. As you mention it would require a separate field being added to the database to expose a unique id per card and would require careful handling for cases where cards are moved across boards. |
lib/Service/ConfigService.php
Outdated
public function isCardIdBadgeEnabled(int $boardId = null): bool | ||
{ | ||
if ($this->getUserId() === null) { | ||
return false; |
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 may not be used anyways, but we could take the $appConfigState
so the admin can set this in case we introduce public shares.
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.
@juliushaertl I am unsure of what you need me to change when you say "take the $appConfigState
". Do you have an example?
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.
@juliushaertl it is the same procedure in the other methods. It can be written so, in my opinion
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.
@juliushaertl @ultrasites either way, if someone could explain what should be changed, I'm happy to do so, otherwise pls resolve this.
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.
Looks good besides some small inline comments and the design feedback discussed earlier.
@juliushaertl Thanks for the feedback.
|
28c6f12
to
166e30a
Compare
Is this a release candidate now? I need it :) |
Yes, let's go with that I'd say. The code seems good otherwise, so we could merge this if you move the CardId to the CardBadges component. 👍 |
480c6d6
to
bfb35a5
Compare
e9ce16d
to
8f65ec2
Compare
@juliushaertl accidentally closed this PR in an attempt to rebase... can you reopen it please? I usually use gitlab and got confused here... |
I can't it seems as the branch does not contain any other commits compared to master. Maybe you can reset the branch to one of the previous commit ids: bfb35a5 |
@juliushaertl @oneWaveAdrian alternatively could you open a new pr? i want to approve it 👍 |
I did reset and force push but it seems the Pr somehow didn't get it. Will open a new one. |
But the pr/repo owner could reopen it via github, or? |
@oneWaveAdrian Just in case you run into issues I still have a local checkout, so I could restore a branch as well. |
@juliushaertl @ultrasites new PR as suggested: #4156 |
Summary
Adds ID Badge to top left corner of card.
Checklist