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

Improve reference provider and add reference widgets #4422

Merged
merged 7 commits into from
Feb 10, 2023

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Feb 1, 2023

The card reference provider is now discoverable and searchable. It supports all the available search providers (boards+cards and comments).

There is a minor flaw (i mean just a weird aspect) of the reference provider design. In this case the provider used to resolve and render cards is supporting all the search providers so we can have one single provider entry in the link picker. I'm fine with that but that can be confusive.

New reference widgets for boards and comments.

There would be a lot of factorization to be done between the card and the comment widget. Just waiting we decide what should be their respective content before factorizing.

Also worth mentioning: I changed the CommentService a bit to make it possible to get a comment from a cardId and a commentId. This lead to a small refactoring of the service.

image

TODO:

  • clarify which icons are used in the widgets

parent::__construct('', $card->getTitle(), $board->getTitle() . ' » ' . $stack->getTitle(), $urlGenerator->linkToRouteAbsolute('deck.page.index') . '#/board/' . $board->getId() . '/card/' . $card->getId(), 'icon-deck');
parent::__construct(
$urlGenerator->getAbsoluteURL(
$urlGenerator->imagePath(Application::APP_ID, 'card.svg')
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if this icon is a better match:

https://pictogrammers.com/library/mdi/icon/card-bulleted-outline/

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 We had the same idea. It's the one I'm using in the reference widget.

@julien-nc julien-nc force-pushed the enh/noid/discoverable-searchable-ref-provider branch 3 times, most recently from df6ec69 to 0859480 Compare February 2, 2023 08:24
@juliusknorr
Copy link
Member

Seems the composer ocp packages are not automatically bumped for deck anymore since we renamed the default branch to main. Filed nextcloud/office#25 for this but feel free to just update that composer package manually for this pr.

Copy link
Member

@marcelklehr marcelklehr left a comment

Choose a reason for hiding this comment

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

Code looks good. Didn't test.

@juliusknorr
Copy link
Member

Integration test failures seem related to search of comments ;)

@julien-nc
Copy link
Member Author

Integration test failures seem related to search of comments ;)

I changed a search provider ID and didn't adjust the tests... All good now.

@julien-nc julien-nc force-pushed the enh/noid/discoverable-searchable-ref-provider branch from 6b5bb6d to b2f01d1 Compare February 5, 2023 23:43
@julien-nc
Copy link
Member Author

Rebased on main

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 👍 Please merge after updating nextcloud/ocp if tests pass :)

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…n ref provider

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…ReferenceProvider (no widgets but resolving)

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…ents

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/noid/discoverable-searchable-ref-provider branch from 60e911c to f775728 Compare February 8, 2023 13:02
@julien-nc
Copy link
Member Author

__construct has more required parameters than parent method for OCA\Deck\Model\BoardSummary and OCA\Deck\Model\CardDetails.

It does not sound like an issue to me 😁. Should we disable/ignore this psalm check?

@juliusknorr
Copy link
Member

Yep, let's ignore that for this PR.

@juliusknorr juliusknorr merged commit fc324f6 into main Feb 10, 2023
@delete-merged-branch delete-merged-branch bot deleted the enh/noid/discoverable-searchable-ref-provider branch February 10, 2023 13:45
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.

3 participants