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.

Sign up for GitHub

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

Ease use of disabled in OcTable & show status icon for processing items #9585

Merged
merged 7 commits into from
Aug 16, 2023
Merged

Ease use of disabled in OcTable & show status icon for processing items #9585

merged 7 commits into from
Aug 16, 2023

Conversation

AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Aug 16, 2023 •
edited
Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@update-docs
Copy link

update-docs bot commented Aug 16, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

AlexAndBear
AlexAndBear commented Aug 16, 2023
View reviewed changes
packages/web-app-files/src/components/SideBar/Shares/Collaborators/EditDropdown.vue
v-if="isExpirationDateSet"
class="oc-ml-s"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

linter

@AlexAndBear AlexAndBear changed the title Ease use of disabled in OcTable Ease use of disabled in OcTable & show status icon for processing items Aug 16, 2023
AlexAndBear
AlexAndBear commented Aug 16, 2023
View reviewed changes
packages/web-client/src/helpers/resource/functions.ts Outdated
@@ -98,7 +98,7 @@ export function buildResource(resource): Resource {
webDavPath: resource.name,
type: isFolder ? 'folder' : resource.type,
isFolder,
processing: resource.processing || false,
processing: !isFolder,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mock

AlexAndBear
AlexAndBear commented Aug 16, 2023
View reviewed changes
packages/design-system/src/components/OcResource/OcResource.vue
@@ -210,6 +210,28 @@ export default defineComponent({

thumbnail() {
return this.resource.thumbnail
},

showStatusIcon() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lookacat service function :) <3

@AlexAndBear AlexAndBear marked this pull request as ready for review August 16, 2023 13:34
JammingBen
JammingBen reviewed Aug 16, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Seeing it now I'm asking myself: did we gain anything by using full resources instead of ids? 🤔 It adds quite a bit of complexity, especially since we already have a full list of resources in OcTable. Now we additionally pass a subset of them to the same component.

packages/design-system/src/components/OcTable/OcTable.vue Outdated Show resolved Hide resolved
@AlexAndBear
Copy link
Contributor Author

Seeing it now I'm asking myself: did we gain anything by using full resources instead of ids? 🤔 It adds quite a bit of complexity, especially since we already have a full list of resources in OcTable. Now we additionally pass a subset of them to the same component.

IMHO it's more handy, developers can just use a subset of the original array and don't need to map those additionally. I don't see more complexity

@AlexAndBear AlexAndBear requested a review from JammingBen August 16, 2023 13:53
@JammingBen
Copy link
Contributor

I mean lines like these are more complex than before: filter((resource) => !this.disabledResources.find((r) => r.id === resource.id)). Plus you can only use and rely on id anyways because items in disabled can literally be anything. So using actual resources from disabled for anything other than id doesn't feel safe 🤷

@AlexAndBear
Copy link
Contributor Author

Well I don't get it, Do you want me to revert it?

@JammingBen
Copy link
Contributor

Nono not completely, but I thought we keep working id based and make the disabled prop in OcTable simply hold an array of ids. And of course remove the disabled prop from ResourceTable like you already did.

@AlexAndBear
Copy link
Contributor Author

Got it 👍

JammingBen
JammingBen approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Noice! 👍

@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

80.8% 80.8% Coverage
1.5% 1.5% Duplication

@AlexAndBear AlexAndBear merged commit 4947174 into master Aug 16, 2023
@delete-merged-branch delete-merged-branch bot deleted the followup-processing branch August 16, 2023 19:11
AlexAndBear pushed a commit that referenced this pull request Dec 13, 2023
@AlexAndBear
Ease use of disabled in OcTable & show status icon for processing ite…
4f83650 
…ms (#9585)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JammingBen JammingBen JammingBen approved these changes

@lookacat lookacat Awaiting requested review from lookacat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlexAndBear @JammingBen