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

Jump to bottom

feat(viewer): add checkbox for every resource in list and grid view (DSP-1711) #311

Merged
merged 17 commits into from Jul 9, 2021
Merged

feat(viewer): add checkbox for every resource in list and grid view (DSP-1711) #311

merged 17 commits into from Jul 9, 2021

Conversation

waychal
Copy link
Contributor

@waychal waychal commented Jun 25, 2021 •
edited

resolves DSP-1711

In this PR, checkbox is added for every resource displayed in list and grid view. It handles 2 cases for both list and grid view:

  • When withMultipleSelection is false (old functionality)

    • if clicked on the single resource, it's details will be displayed

  • When withMultipleSelection is true (new functionality)

    • if clicked on the single resource, it's details will be displayed
    • if checkboxes are selected, to will be redirected to the comparison viewer

@waychal waychal added the enhancement New feature or request label Jun 25, 2021
@waychal waychal self-assigned this Jun 25, 2021
@kilchenmann kilchenmann changed the title feat(resource comparison viewer): add checkbox for every resource in list and grid view (DSP-1711) feat(viewer): add checkbox for every resource in list and grid view (DSP-1711) Jun 25, 2021
@kilchenmann kilchenmann changed the title feat(viewer): add checkbox for every resource in list and grid view (DSP-1711) feat(viewer)!: add checkbox for every resource in list and grid view (DSP-1711) Jun 25, 2021
@kilchenmann kilchenmann added the breaking Breaking change label Jun 25, 2021
@waychal waychal requested a review from kilchenmann June 28, 2021 13:55
@waychal
Copy link
Contributor Author

waychal commented Jun 28, 2021

@kilchenmann I have added the interface and also moved the checkbox to the top right corner. As you said, we can work on the grid template separately to refine it further.

@kilchenmann
Copy link
Contributor

@kilchenmann I have added the interface and also moved the checkbox to the top right corner. As you said, we can work on the grid template separately to refine it further.

I will have a look at it tomorrow morning.

@kilchenmann
Copy link
Contributor

Yesterday evening I was thinking about this multiple resource selection. Why not adding an additional @Input parameter where the user can say withMultipleSelection (true/false). By default it's false. This will help to avoid a breaking change.

@waychal
Copy link
Contributor Author

waychal commented Jun 29, 2021

Yesterday evening I was thinking about this multiple resource selection. Why not adding an additional @Input parameter where the user can say withMultipleSelection (true/false). By default it's false. This will help to avoid a breaking change.

@kilchenmann It is good idea. I can update the PR.
@subotic can you please comment here that it is fine?

@subotic
Copy link

subotic commented Jun 29, 2021

@waychal yes, it is fine.

@waychal waychal marked this pull request as draft July 2, 2021 12:06
@waychal waychal changed the title feat(viewer)!: add checkbox for every resource in list and grid view (DSP-1711) feat(viewer): add checkbox for every resource in list and grid view (DSP-1711) Jul 2, 2021
@waychal waychal removed the breaking Breaking change label Jul 2, 2021
@waychal waychal marked this pull request as ready for review July 2, 2021 22:51
@waychal
Copy link
Contributor Author

waychal commented Jul 2, 2021

@kilchenmann I have updated the code to handle both cases of single and multiple resource selection.

Snehal Kumbhar added 2 commits July 5, 2021 14:37
feat(resource viewer): added deprecated warning for resourceSelected …
aab1dbc 
…event
feat(resource viewer): update multiple selection case
88c534b 
- when withMultipleSelection is true and clicked on the resource, it will go
  to the resource details page
- when withMultipleSelection is true and checkbox(s) is selected, it will go to
  the resource comparison page

- resource-grid-content component is deleted
- resource-list-content component is deleted
@waychal waychal requested a review from kilchenmann July 6, 2021 14:22
kilchenmann
kilchenmann reviewed Jul 7, 2021
View reviewed changes
Copy link
Contributor

@kilchenmann kilchenmann left a comment

Choose a reason for hiding this comment

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

It works. But there are some little details that have to be fixed first

projects/dsp-ui/src/lib/viewer/views/list-view/resource-list/resource-list.component.html
Comment on lines +11 to +14
<p matLine class="res-class-label">
{{ resource.entityInfo.classes[resource.type].label }}
</p>
<h3 matLine class="res-class-value">{{ resource.label }}</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you set the style for res-class-label and res-class-value to margin: 0;? Otherwise it needs too much space.

Copy link
Contributor Author

@waychal waychal Jul 7, 2021 •
edited

Choose a reason for hiding this comment

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

It is the case only on big screens. margin:0 removes all the space and it looks crowded. I will make it margin: 5px 0, if it ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also on small screen to height:
Screen Shot 2021-07-07 at 12 52 45

And this is how it looks with margin: 0:
Screen Shot 2021-07-07 at 12 56 34

If you want to add a margin, we should follow to rule of 8. In this case you could set it to margin: 4px 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looks like this on my machine with current style.

image

Anyways I will update the margin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... that's weird 😳 it looks different on the two machines. But why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we using same version? I took this screenshot from https://admin.test.dasch.swiss/ server. I did not run the DSP-APP locally to check it there.

For dsp-ui-lib server running locally, it looks like below. It does show some spacing. So I assumed that in DSP-APP there are some extra styles applied for this as I did not change this style in this PR until now.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I added the DSP-UI lib from your branch to DSP-APP locally (with yalc) and was running your implementation there. The version on https://admin.test.dasch.swiss/ doesn't have your implementation. Of course it looks different 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

In your screenshot you have the same big spaces that I have in DSP-APP. This is why I wrote to set the margin to 0 pixels.

projects/dsp-ui/src/lib/viewer/views/list-view/list-view.component.ts Outdated Show resolved Hide resolved
projects/dsp-ui/src/lib/viewer/views/list-view/list-view.component.ts Show resolved Hide resolved
@waychal waychal requested a review from kilchenmann July 7, 2021 11:51
@waychal
Copy link
Contributor Author

waychal commented Jul 9, 2021

@kilchenmann @mdelez can you please review the PR.

kilchenmann
kilchenmann approved these changes Jul 9, 2021
View reviewed changes
Copy link
Contributor

@kilchenmann kilchenmann left a comment

Choose a reason for hiding this comment

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

Thanks for all the updates. Works perfect 👍

@waychal waychal merged commit 1e0381a into main Jul 9, 2021
@waychal waychal deleted the wip/DSP-1711-add-checkbox-for-every-resource branch July 9, 2021 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kilchenmann kilchenmann kilchenmann approved these changes

@mdelez mdelez Awaiting requested review from mdelez

Assignees

@waychal waychal

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@waychal @kilchenmann @subotic