-
Notifications
You must be signed in to change notification settings - Fork 356
feat: managing project versions #4811
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
base: main
Are you sure you want to change the base?
Conversation
apps/frontend/src/components/ui/create-project-version/MultiStageModal.vue
Outdated
Show resolved
Hide resolved
apps/frontend/src/components/ui/create-project-version/CreateProjectVersionModal.vue
Show resolved
Hide resolved
…ys being "required"
IMB11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good! Just a few nitpicks and changes that could be considered
| import AddLoadersStage from './stages/AddLoadersStage.vue' | ||
| import AddMcVersionsStage from './stages/AddMcVersionsStage.vue' | ||
| const { newDraftVersion, draftVersion, detectedLoaders, detectedVersions, projectType } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might better suited for DI? - see creator-withdraw.ts for an example.
| const modal = useTemplateRef<InstanceType<typeof MultiStageModal>>('modal') | ||
| provide('createVersionModal', modal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should go through our DI framework - could merge with the manage stuff
| text: 'The version has been successfully added to your project.', | ||
| type: 'success', | ||
| }) | ||
| // TODO: refetch versions here for project versions table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use tanstack mutations for this - https://tanstack.com/query/v5/docs/framework/vue/guides/mutations
| window.addEventListener('keydown', handleKeyDown) | ||
| window.addEventListener('keyup', handleKeyUp) | ||
| }) | ||
| onUnmounted(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const searchLoading = ref(false) | ||
| const options = ref<DropdownOption<string>[]>([]) | ||
| const client = injectModrinthClient() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const client = injectModrinthClient() | |
| const { labrinth } = injectModrinthClient() |
| * const results = await client.labrinth.projects_v2.search({ | ||
| * query: 'optimization', | ||
| * facets: [['categories:optimization'], ['project_type:mod']], | ||
| * facets: "[['categories:optimization'], ['project_type:mod']]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see prev comment about typing this properly
| } | ||
|
|
||
| /** | ||
| * Request data object for Modrinth “Create Version” API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Sounds counter intuitive but we haven't usually done comments for these types because the backend has comments already/it'll probably go out of date over time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aecsocket is working on slowly adding utoipa support on labrinth, like is already done for some internal services
| @@ -0,0 +1,238 @@ | |||
| import type { DraftVersion } from '../../../../../../apps/frontend/src/composables/versions/manage-version' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont do this - put DraftVersion into api-client types
| import { useRoute, useRouter } from 'vue-router' | ||
| import { PlusIcon } from '@modrinth/assets' | ||
| import { ButtonStyled } from '@modrinth/ui' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we try not to use @modrinth/ui self-imports because it can sometimes cause cyclic dependencies as some of the older components are exported a bit weirdly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so instead better to import directly like this?
import ButtonStyled from '@modrinth/ui/src/components/base/ButtonStyled.vue'
| @@ -0,0 +1,267 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be in packages/tooling-config though.
Feature:
Added:
MultiStageModalcomponentDropzoneFileInputcomponentfolder-upicon