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

Separate Create Profile modal from Profiles.vue #1355

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

VilppeRiskidev
Copy link
Collaborator

  • TODO: clean up old create profile stuff from Profiles.vue, which is easier to do with the refactoring of the Import module as the two were somewhat convoluted.

Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

Didn't do a proper review yet, but the start seems as it should 👍🏻

One thing you might want to consider, is to centralize the methods shared between multiple profile modals, e.g. get profileList, makeProfileNameSafe, doesProfileExist. See e.g. how Profiles.vue uses ProfileMixin.vue. That being said, the mixin adds a bit of complexity and requires jumping between files when reading the code, so it might not be worth it if only few methods are duplicated between two files. (On the other hand it's quite nice from maintenance perspective if r2mm and tsmm have different implementations of the component that uses the mixin, like Profiles does.)

Base automatically changed from rename-profile-modal-refactor to develop June 11, 2024 05:36
Copy link
Collaborator

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

Few observations, hopefully quick to fix.

src/components/profiles-modals/CreateProfileModal.vue Outdated Show resolved Hide resolved
src/components/profiles-modals/CreateProfileModal.vue Outdated Show resolved Hide resolved
</template>

<template v-slot:body>
<p>This profile will store its own mods independently from other profiles.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems something is off style-wise. The input no longer has vertical white space, causing this help text and the validation tags below cling to it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally some scoped CSS would've been used, but I guess this works too.

- TODO: clean up old create profile stuff from Profiles.vue,
 which is easier to do with the refactoring of the Import module
 as the two were somewhat convoluted.
@anttimaki anttimaki marked this pull request as ready for review August 14, 2024 14:37
@anttimaki anttimaki merged commit ac94111 into develop Aug 29, 2024
5 of 7 checks passed
@anttimaki anttimaki deleted the create-profile-modal-refactor branch August 29, 2024 12:56
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.

2 participants