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

refactor: remove opinionated components from ODS #10293

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Jan 4, 2024

Description

Removes the following opinionated components from the ODS and moves them to web-pkg instead:

  • OcResource (now ResourceListItem)
  • OcTile (now ResourceTile)
  • OcResourceIcon (now ResourceIcon)
  • OcGhostElement (now ResourceGhostElement)
  • OcResourceLink (now ResourceLink)
  • OcResourceName (now ResourceName)
  • OcResourceSize (now ResourceSize)

Those components hold logic about resources, which is something that is not supposed to live in the ODS. The ODS should only contain design- and layout-specific components.

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
  • Documentation
  • Maintenance (e.g. dependency updates or tooling)

@JammingBen JammingBen self-assigned this Jan 4, 2024
@JammingBen JammingBen force-pushed the move-opinionated-resource-components-to-pkg branch 2 times, most recently from b04e7f3 to c09a296 Compare January 5, 2024 07:27
@JammingBen JammingBen marked this pull request as ready for review January 5, 2024 09:49
packages/web-pkg/src/components/DragGhostElement.vue Outdated Show resolved Hide resolved
packages/web-pkg/src/components/ResourceListItem.vue Outdated Show resolved Hide resolved
packages/web-pkg/src/components/ResourceIcon.vue Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Move into subfolder FilesList? The ResourceTiles.vue component still lives in the files app, but it would make sense to move it here into the FilesList folder anyway... if you think the scope of this PR can handle that move as well, then go for it 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving ResourceTiles.vue is a little more work to do, I created an issue for this: #10304

packages/web-pkg/src/helpers/resource/icon.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this component now moved from the (potentially standalone) design system into web-pkg we don't need to provide/inject the icon mappings anymore. Instead the component could query that from the runtime / a composable / whatever. But maybe something for a followup... could you create an issue? Or just add a todo in your PR description, so that I can create the issue before we merge and after we agreed that we want to change it. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I've created #10305 for this.

@dschmidt
Copy link
Member

dschmidt commented Jan 8, 2024

DragGhostElement - I think this should somehow also contain Resource in the name

@JammingBen
Copy link
Contributor Author

DragGhostElement - I think this should somehow also contain Resource in the name

I decided for ResourceGhostElement, IMO we don't necessarily need the information "drag" here.

Removes the following opinionated components from the ODS and moves them to `web-pkg` instead:

- `OcResource` (now `ResourceListItem`)
- `OcTile` (now `ResourceTile`)
- `OcResourceIcon` (now `ResourceIcon`)
- `OcGhostElement` (now `DragGhostElement`)

Those components hold logic about resources, which is something that is not supposed to live in the ODS. The ODS should only contain design- and layout-specific components.
Move the following components to web-pkg:

- `OcResourceLink` (now `ResourceLink`)
- `OcResourceName` (now `ResourceName`)
- `OcResourceSize` (now `ResourceSize`)
@JammingBen JammingBen force-pushed the move-opinionated-resource-components-to-pkg branch from da34479 to e43c9f8 Compare January 8, 2024 13:05
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Nice! Now my last request for change: remove web-client from the dependency list please 😅

Copy link

sonarqubecloud bot commented Jan 8, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

11 New issues
0 Security Hotspots
72.5% Coverage on New Code
0.5% Duplication on New Code

See analysis details on SonarCloud

@JammingBen JammingBen requested a review from kulmann January 8, 2024 14:35
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Hooray, thank you 🥳

@kulmann kulmann merged commit ea8081d into master Jan 8, 2024
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the move-opinionated-resource-components-to-pkg branch January 8, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants