Skip to content
This repository has been archived by the owner on Aug 28, 2024. It is now read-only.

Convert many components to Composition API #100

Closed

Conversation

Mysterious-Dev
Copy link
Contributor

@Mysterious-Dev Mysterious-Dev commented Oct 14, 2023

All is in the title ^^.

Components converted :

  • Avatar
  • CopyCode
  • CheckBox
  • EnvironmentIndicator
  • Pagination

@Mysterious-Dev Mysterious-Dev changed the title Convert Avatar & CopyCode components to Composition API Convert Avatar, CheckBox & CopyCode components to Composition API Oct 15, 2023
@Mysterious-Dev Mysterious-Dev changed the title Convert Avatar, CheckBox & CopyCode components to Composition API Convert many components to Composition API Oct 15, 2023
@Mysterious-Dev
Copy link
Contributor Author

Merge conflicts fixed ^^.

@Mysterious-Dev
Copy link
Contributor Author

@brawaru I've applied your suggestions ^^.

@brawaru
Copy link
Contributor

brawaru commented Oct 28, 2023

Would you be okay with splitting this pull request into several smaller pull requests per component? Would be easier to review and merge at least some of these changes, rather than trying merge everything all at once.

@Mysterious-Dev
Copy link
Contributor Author

Would you be okay with splitting this pull request into several smaller pull requests per component? Would be easier to review and merge at least some of these changes, rather than trying merge everything all at once.

No worries, I'll take care of it over the weekend. It's true that it will be easier to review ^^.

@triphora
Copy link
Contributor

I think it's fine to be one PR, it's only four files.

@@ -49,66 +49,63 @@
</a>
</div>
</template>
<script setup>
<script setup lang="ts">
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, a separate PR for doing typescript conversions separate from the composition API conversions would be nice.

Copy link
Contributor

@brawaru brawaru Oct 28, 2023

Choose a reason for hiding this comment

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

I think split on components makes a lot more sense because the PRs will atomic and independent, thus can be accepted quicker (as long as you are going to review and merge).

Split on Composition and TypeScript conversions makes less sense, since TypeScript conversion achieves the same goal that Composition conversion achieves. E.g., defineProps with TypeScript generic emits the same code that defineProps with options argument.

It's just twice the effort. There's no crazy TypeScript shenanigans going on to really justify a separate review. But it's up to you.

@Mysterious-Dev
Copy link
Contributor Author

Splited into different PR, #124, #125, #126 & #127

@Mysterious-Dev Mysterious-Dev deleted the composition-api-1 branch October 29, 2023 10:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants