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

feature: remove all scope ( V2 ) #2323

Merged
merged 3 commits into from
Jan 16, 2024
Merged

feature: remove all scope ( V2 ) #2323

merged 3 commits into from
Jan 16, 2024

Conversation

Paul-Bob
Copy link
Contributor

@Paul-Bob Paul-Bob commented Jan 4, 2024

Description

Fixes #2276

Check https://github.com/avo-hq/avo-advanced/pull/25 for details

@Paul-Bob Paul-Bob self-assigned this Jan 4, 2024
Copy link

codeclimate bot commented Jan 4, 2024

Code Climate has analyzed commit 1f4fa77 and detected 0 issues on this pull request.

View more on Code Climate.

@@ -1,7 +1,7 @@
module Avo
module Loaders
class Loader
attr_accessor :bag
attr_accessor :bag, :remove_scope_all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's extract this to a separate ScopeLoader class.

@@ -164,7 +164,8 @@ def scopes_list
resource: resource,
turbo_frame: turbo_frame,
parent_record: parent_record,
query: query
query: query,
remove_scope_all: resource.entity_loader(:scope).remove_scope_all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we pass the loader and have the component ask the loader if it should show the default scope or not?

I feel that adding arbitrary options to it every time we change the API is not the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can and it's better for scalability

@Paul-Bob Paul-Bob merged commit 4cbf642 into main Jan 16, 2024
28 checks passed
@Paul-Bob Paul-Bob deleted the feature/remove_all_scope_v2 branch January 16, 2024 10:59
Copy link
Contributor

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to remove the "All" default scope
2 participants