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

feat: #2906 add card ID badge #3757

Closed

Conversation

oneWaveAdrian
Copy link
Contributor

@oneWaveAdrian oneWaveAdrian commented Apr 26, 2022

  • adds vscode settings file to gitingore
  • adds new badge for card ID
  • adds card ID to board filter
  • adds settings to disable card ID badge

Summary

Adds ID Badge to top left corner of card.
image

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

@oneWaveAdrian
Copy link
Contributor Author

@juliushaertl can this be marked as to review please?

Copy link
Member

@stefan-niedermann stefan-niedermann left a 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)?

@juliusknorr
Copy link
Member

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

@oneWaveAdrian
Copy link
Contributor Author

@juliushaertl that is why I placed it above the title, already tested with up to 12 digits and it works without impact.
@stefan-niedermann I'd appreciate design input, see this as a technical draft, happy to edit to fit in. I felt an additional badge took too much space in the grand scheme, so I looked at how Jira and Trello do it.
@juliushaertl setting for hiding this feature is already implemented, I am unsure of what you mean.

@raimund-schluessler
Copy link
Member

@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?

@oneWaveAdrian
Copy link
Contributor Author

oneWaveAdrian commented Apr 28, 2022

@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 uid in the first place.

If we look at other popular tools:

So their card numbering is depending on the board and thus gets a global proper uid rather than a global id value. Making the assumption that a board itself does not accumulate a 12 digit amount of cards over time.


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.

@nimishavijay
Copy link
Member

Super nice work! :)

I'd say we can add it to the list of card badges on the bottom left

This seems like a good idea! It could go in the beginning of the badges, something like
image

ID = OWSWS-422

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.
Right now:

  • Board1 has cards with ID 1, 2, 3, 4, 5
  • Board2 has cards with ID 6, 7, 8

Instead we could do

  • Board1 has cards with ID 1, 2, 3, 4, 5
  • Board2 has cards with ID 1, 2, 3

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 #1. What do you think?

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.

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?

@oneWaveAdrian
Copy link
Contributor Author

4 days later - How are we going to proceed here?

@oneWaveAdrian
Copy link
Contributor Author

@jancborchardt, any updates from your end?

@jancborchardt
Copy link
Member

Hi, sorry @oneWaveAdrian 👍’d @nimishavijay’s proposal but let me expand:

  • Agree with @juliushaertl’s proposal of putting it on the bottom row, and @nimishavijay’s mockup for that is great
  • Agree with your assessment @oneWaveAdrian that it should be a setting disabled by default

However, there’s the big confusion about the usefulness of the ID as @juliushaertl mentioned:

Note that the id is unique across users/boards

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 #1 for each board.

@oneWaveAdrian
Copy link
Contributor Author

However, there’s the big confusion about the usefulness of the ID as @juliushaertl mentioned:

Note that the id is unique across users/boards

@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 #1) and still have the global ID. Therefore nothing it will remain backwards compatible and nothing changes for those users who won't need it.

@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 #9999? Will that still work design wise/is there enough space?

@nimishavijay
Copy link
Member

What happens if an ID on a boarddoes get up to let's say #9999? Will that still work design wise/is there enough space?

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)

@oneWaveAdrian
Copy link
Contributor Author

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.

@oneWaveAdrian
Copy link
Contributor Author

one Month later - what's the decision on this @juliushaertl @jancborchardt @nimishavijay @stefan-niedermann?

@stefan-niedermann

This comment was marked as off-topic.

@oneWaveAdrian
Copy link
Contributor Author

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

@juliusknorr juliusknorr self-requested a review August 9, 2022 05:51
@juliusknorr
Copy link
Member

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.

public function isCardIdBadgeEnabled(int $boardId = null): bool
{
if ($this->getUserId() === null) {
return false;
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link

@ultrasites ultrasites Oct 20, 2022

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

Copy link
Contributor Author

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.

Copy link
Member

@juliusknorr juliusknorr left a 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.

@juliusknorr juliusknorr mentioned this pull request Aug 9, 2022
5 tasks
@oneWaveAdrian
Copy link
Contributor Author

@juliushaertl Thanks for the feedback.
Just to be sure:
I am placing the ID in the bottom left as suggested by @nimishavijay, even though this could create visual problems if the ID was too long, correct?

I'd say we can add it to the list of card badges on the bottom left

This seems like a good idea! It could go in the beginning of the badges, something like image

@ultrasites
Copy link

Is this a release candidate now? I need it :)

.gitignore Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member

I am placing the ID in the bottom left as suggested by @nimishavijay, even though this could create visual problems if the ID was too long, correct?

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. 👍

@oneWaveAdrian
Copy link
Contributor Author

@juliushaertl accidentally closed this PR in an attempt to rebase... can you reopen it please? I usually use gitlab and got confused here...

@juliusknorr
Copy link
Member

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

@ultrasites
Copy link

@juliushaertl @oneWaveAdrian alternatively could you open a new pr? i want to approve it 👍

@oneWaveAdrian
Copy link
Contributor Author

I did reset and force push but it seems the Pr somehow didn't get it. Will open a new one.

@ultrasites
Copy link

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?

@juliusknorr
Copy link
Member

Not if the source branch has no commits:

Screenshot 2022-10-26 at 13 44 03

@juliusknorr
Copy link
Member

@oneWaveAdrian Just in case you run into issues I still have a local checkout, so I could restore a branch as well.

@oneWaveAdrian
Copy link
Contributor Author

@juliushaertl @ultrasites new PR as suggested: #4156
It is weird, the reason this happened was that I clicked on "Synch Fork" in my branch repo and then it deleted all my edits as it reset to master apparently... will do it through terminal again next time, thought I'd try this github feature to save time, oh well...

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.

IDs for each card
7 participants