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

profile menu started #101

Closed
wants to merge 6 commits into from
Closed

Conversation

belljohan3
Copy link
Collaborator

No description provided.

@belljohan3 belljohan3 linked an issue Apr 29, 2023 that may be closed by this pull request
4 tasks
@belljohan3 belljohan3 self-assigned this Apr 29, 2023
components/AppToolbar.vue Outdated Show resolved Hide resolved
components/DropdownMenu.vue Outdated Show resolved Hide resolved
components/DropdownMenu.vue Outdated Show resolved Hide resolved
const onError = (error: any) => {
console.error(error);
};
const onError = (error: any) => error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for this change?
With this change the onError function only returns the error we get from the NuxtErrorBoundary, but we dont do anything with it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made these changes to avoid getting the workflow errors! when I do the git push

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert these changes. They are reported as warning by eslint because we use console, but its not an error and this shouldn't stop you from contributing.

@sifferhans Related to #33 - we should define a strategy of how to deal with errors of various kinds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nuxt has a NuxtErrorBoundary component that catches errors from child components, which has a @error event. We could look into using this for client side errors, but we probably need a separate strategy for API errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sifferhans See #33 (comment). We should move this discussion, its off topic here.

@SimonSimCity
Copy link
Collaborator

SimonSimCity commented May 2, 2023

@sifferhans IMO some of your comments should have gone to a different PR. Most of the changes here were sorely done to reduce the amount of warnings eslint spits out, mostly https://eslint.vuejs.org/rules/attributes-order.html. I've been so free to close these conversations.

Because you asked: The event propagation is needed for the click event not to bubble up to the track item, which the dropdownMenu component is a part of, which in turn has a different click event. See https://developer.mozilla.org/en-US/docs/Web/API/Event/stopPropagation for further information. IMO, the function stopPropagation() shouldn't be called inside a function, because its then hidden from the HTML-structure because of which it is called. IMO, we should take a turn on all events and call event.stopPropagation(); myFunction(event) if possible, to know where the propagation is done and where its not done. Sometimes its also unwanted to stop the propagation - and in those cases its better to have the code where its important to know about the side-effects. But this is more of me going into the nature of functional programming. Comments welcome 🤗

Copy link
Collaborator

@SimonSimCity SimonSimCity left a comment

Choose a reason for hiding this comment

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

Just these three changes and I'm fine:

  • eslint rule
  • light tag
  • error-handling of NuxtErrorBoundary

components/AppToolbar.vue Outdated Show resolved Hide resolved
const onError = (error: any) => {
console.error(error);
};
const onError = (error: any) => error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert these changes. They are reported as warning by eslint because we use console, but its not an error and this shouldn't stop you from contributing.

@sifferhans Related to #33 - we should define a strategy of how to deal with errors of various kinds.

Copy link
Collaborator

@SimonSimCity SimonSimCity left a comment

Choose a reason for hiding this comment

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

Would be nice to get this PR merged soon. Please take a look at the open points and discuss or change where necessary.

package.json Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated
@@ -1,3 +1,4 @@
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like you have not resolved all the merge-conflicts 😅 This file can be reverted and regenerated by running pnpm install...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The conflict has been resolved now.

class="absolute right-0 mt-2 w-56 origin-top-right divide-y divide-gray-100 rounded-md bg-white shadow-lg ring-1 ring-black ring-opacity-5 focus:outline-none"
>
<div class="px-1 py-1">
<MenuItem v-slot="{ active }">
Copy link
Collaborator

Choose a reason for hiding this comment

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

All those menu items seem to have the same property. Can't we use a loop here..?

I'd only include options which are done yet or show some kind of message - not something which we are about to implement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just look at the headlessui documentation. I would like also to refact it to have less code. Would you let me know how to do it ?

Also, i've installed nuxt-headlessui module and set it up in nuxt.config.ts file. But i am still getting red line when i am trying to import elements i need for the menu

@belljohan3 belljohan3 removed a link to an issue May 19, 2023
4 tasks
@belljohan3 belljohan3 closed this May 19, 2023
@belljohan3 belljohan3 deleted the 8-profile-dropdown-storing-settings branch May 19, 2023 10:40
@belljohan3 belljohan3 restored the 8-profile-dropdown-storing-settings branch May 19, 2023 10:40
@belljohan3 belljohan3 deleted the 8-profile-dropdown-storing-settings branch May 19, 2023 10:40
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.

3 participants