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

feat: add threat shield dns #479

Merged
merged 6 commits into from
Jan 20, 2025
Merged

feat: add threat shield dns #479

merged 6 commits into from
Jan 20, 2025

Conversation

andre8244
Copy link
Collaborator

@andre8244 andre8244 commented Jan 9, 2025

Add Threat shield DNS UI

Ref:

@andre8244 andre8244 force-pushed the threat-shield-dns branch 4 times, most recently from 8d33f7a to addeb82 Compare January 17, 2025 15:10
@andre8244 andre8244 marked this pull request as ready for review January 17, 2025 15:13
@andre8244 andre8244 requested a review from Tbaile January 17, 2025 15:14
Copy link
Collaborator

@Tbaile Tbaile left a comment

Choose a reason for hiding this comment

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

I'll leave some comments here due to the size of the PR, however a few things might need some discussion:

  • Import styles are mixed, some import are using relative paths, some imports leverage the @/ syntax, I'd rather start choosing one of the two going forward
  • (personal preference) I'd start to use the TS oriented defineProps, and use the props destructuring to ease the usage of the props. prefix for many variables
  • Unless we need comparison between states of a watched elements, I'd convert watch statements with its new watchEffect`, avoids variable drilling of unknown type and lets Vue do it's magic

package-lock.json Show resolved Hide resolved
src/stores/standalone/threatShield.ts Show resolved Hide resolved
@andre8244
Copy link
Collaborator Author

Import styles are mixed, some import are using relative paths, some imports leverage the @/ syntax, I'd rather start choosing one of the two going forward

I usually rely on my IDE's automatic import function. Manually reviewing every import feels quite tedious if it's just for cosmetic reasons.. However, if you have a semi-automated solution in mind (like a linter), we can implement it

@andre8244
Copy link
Collaborator Author

I'd start to use the TS oriented defineProps, and use the props destructuring to ease the usage of the props. prefix for many variables

I can agree on this one, even if it's just cosmetics

@andre8244
Copy link
Collaborator Author

Unless we need comparison between states of a watched elements, I'd convert watch statements with its new watchEffect`, avoids variable drilling of unknown type and lets Vue do it's magic

I agree, but not 100%. While watchEffect is a great feature, it can make it less clear why that code block exists in the first place. Which approach is better likely depends on the specific use case.

@Tbaile
Copy link
Collaborator

Tbaile commented Jan 20, 2025

I usually rely on my IDE's automatic import function.

Figured, but something might need some adjustments in the settings, might give a look when I have time (since VSCode parses tsconfig no issues, still a weird behaviour). I've just learned that the "Quick Fix" function is different from the "Import" one, not sure this can be even addressed properly 🙄

if you have a semi-automated solution in mind (like a linter), we can implement it

Not sure it can be done, sadly.
No hurt tho, was just a thing that I've noticed

While watchEffect is a great feature, it can make it less clear why that code block exists in the first place

Sure, but then I'd rather use the returning watched element in the function, instead of reusing the referenced element again, this sure saves us from side-effects of accidentally update the variable that is being watched

@andre8244
Copy link
Collaborator Author

Sure, but then I'd rather use the returning watched element in the function, instead of reusing the referenced element again, this sure saves us from side-effects of accidentally update the variable that is being watched

Did not understand what you mean here

@Tbaile
Copy link
Collaborator

Tbaile commented Jan 20, 2025

For example, turning this

watch(
  () => props.isShown,
  () => {
    if (props.isShown) {
      clearErrors()
      focusElement(bypassRef)
      bypass.value = ''
    }
  }
)

Into this

watch(
  () => props.isShown,
  (shown) => {
    if (shown) {
      clearErrors()
      focusElement(bypassRef)
      bypass.value = ''
    }
  }
)

@Tbaile Tbaile self-requested a review January 20, 2025 11:25
@andre8244 andre8244 merged commit 8513354 into main Jan 20, 2025
3 checks passed
@andre8244 andre8244 deleted the threat-shield-dns branch January 20, 2025 11:37
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