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

Fix: Refactored App.vue #166

Merged
merged 34 commits into from
May 15, 2023
Merged

Fix: Refactored App.vue #166

merged 34 commits into from
May 15, 2023

Conversation

BeierKevin
Copy link
Contributor

@BeierKevin BeierKevin commented May 11, 2023

PR description

Describe your changes in detail here

The App.vue file was way to large, it was hard to read and understand. To make it more clear what is happening for others to understand i extracted parts of it into other components to make it a lot easierer to understand and read.

Definition Of Done (DoD)

This PR can be squashed / merged if

  • a developer is assigned
  • the PR is NOT estimated
  • the PR is labeled
  • the PR is NOT assigned to the current sprint
  • a meaningful title has been set according to https://www.conventionalcommits.org/
  • the PR is described in detail
  • the PR links to an issue
  • the PR has been reviewed

Add additional conditions here if necessary for this PR

Fix: #160
Fix: #162

@BeierKevin BeierKevin added Phase: Construction RUP: Implementation improvement Some layout / structure / performance optimization labels May 11, 2023
@BeierKevin BeierKevin self-assigned this May 11, 2023
@BeierKevin
Copy link
Contributor Author

For some arbitrary reason, after refactoring the token generation, the GitHub Actions CI can't find the crypto module.

@BeierKevin BeierKevin requested a review from Claiyc May 12, 2023 11:19
@BeierKevin BeierKevin marked this pull request as ready for review May 12, 2023 11:23
Copy link
Member

@Claiyc Claiyc left a comment

Choose a reason for hiding this comment

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

Very nice work overall, much better than before!

src/components/SettingsPrompt/SettingsPrompt.vue Outdated Show resolved Hide resolved
src/composables/useClipboard/useClipboard.test.ts Outdated Show resolved Hide resolved
src/composables/useClipboard/useClipboard.ts Outdated Show resolved Hide resolved
@BeierKevin BeierKevin mentioned this pull request May 13, 2023
6 tasks
@Claiyc
Copy link
Member

Claiyc commented May 13, 2023

tests failing?

@BeierKevin
Copy link
Contributor Author

Indeed cuz of the node environment the test is done it cant find or use the package for clipboard on a node environment

@BeierKevin
Copy link
Contributor Author

I made a simple workaround, because for some reason the node version of GitHub won't access the clipboard or something.

@BeierKevin
Copy link
Contributor Author

#166 (comment)

What should I change there ?

@Claiyc
Copy link
Member

Claiyc commented May 14, 2023

#166 (comment)

What should I change there ?

I'd implement a loop here that generates ~10 tokens and checks for each of them if they meet all rules

@BeierKevin BeierKevin requested a review from Claiyc May 15, 2023 14:14
Copy link
Member

@Claiyc Claiyc left a comment

Choose a reason for hiding this comment

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

lgtm, good work

@BeierKevin BeierKevin merged commit a2eb477 into main May 15, 2023
@BeierKevin BeierKevin deleted the fix/160-refactor-app.vue branch May 15, 2023 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Some layout / structure / performance optimization Phase: Construction RUP: Implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor with design pattern Refactor App.vue
2 participants