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

Refactor FEmptyWishlist/FExportWishlist #2051

Conversation

candela97
Copy link
Collaborator

  1. Refactor the export wishlist dialog to use the new builder.
  2. Merge FEmptyWishlist and FExportWishlist into a single feature so we can easily manage feature order and button styles. I used button instead of div elements to avoid a11y issues, but I think it looks alright.

@tfedor
Copy link
Member

tfedor commented Sep 16, 2024

I'm not really sure what I think of merging those two into one feature. Was feature order and button styles ever an issue?
For button we should use common components/styles, feature order should not matter because those features are independent.

The entire feature system we have was, I believe, meant to help with keeping independent features independent and if we continue this way we could very easily end up having everything in just context class.

The new FWishlistActionButtons feature is also much less descriptive and could do a lot of things, instead of just FEmptyWishlist and FExport Wishlist.

@candela97
Copy link
Collaborator Author

Feature order matters because these features anchor on to the same node, and changing the order will mess up the spacing, that's why there's a dependency. But ok I'll leave it for now since creating a component for 2 buttons doesn't seem worth it.

@candela97 candela97 closed this Sep 16, 2024
@tfedor
Copy link
Member

tfedor commented Sep 16, 2024

Feature order matters because these features anchor on to the same node, and changing the order will mess up the spacing, that's why there's a dependency. But ok I'll leave it for now since creating a component for 2 buttons doesn't seem worth it.

This is already solved with dependency system, or, if we didn't want to do that, it could be fixed with selector change in these features.

@candela97
Copy link
Collaborator Author

I meant to get rid of the dependency since it only affects visuals and is better done with svelte. There was a similar situation with 3rd-party info on app pages before, and we used to have logic in the content class that inserted anchors to the page before features started loading, how does that sound?

@tfedor
Copy link
Member

tfedor commented Sep 16, 2024

I meant to get rid of the dependency since it only affects visuals and is better done with svelte.

Right now these features are only dependent because of order of buttons, right? So we can control that with changing selector when applying feature, instead of using dependency system.

There was a similar situation with 3rd-party info on app pages before, and we used to have logic in the content class that inserted anchors to the page before features started loading, how does that sound?

I don't think it's necessary to add anchors for this to work to be honest.

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