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

Add disable_all_items in View #1199

Merged
merged 23 commits into from
Apr 29, 2022
Merged

Conversation

27Saumya
Copy link
Contributor

@27Saumya 27Saumya commented Mar 26, 2022

Summary

  • disable_all_items function in View which disables all items. It has a kwarg (exclusions)(Optional[List[int]]) which ignores the specific index of self.children and disables the rest children.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, typehinting, examples, ...)

Copy link
Member

@Middledot Middledot left a comment

Choose a reason for hiding this comment

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

I think it would make sense to also support lists of Item objects into the exclusions param.

discord/ui/view.py Outdated Show resolved Hide resolved
Co-authored-by: Middledot <78228142+Middledot@users.noreply.github.com>
discord/ui/view.py Outdated Show resolved Hide resolved
Co-authored-by: krittick <ben@krittick.net>
Lulalaby
Lulalaby previously approved these changes Apr 11, 2022
@Lulalaby Lulalaby enabled auto-merge (squash) April 11, 2022 08:25
@Lulalaby Lulalaby requested review from krittick and Middledot April 11, 2022 08:25
@krittick
Copy link
Contributor

I think it would make sense to also support lists of Item objects into the exclusions param.

@27Saumya can you please implement this suggestion? Being able to pass the actual Item objects to exclude rather than only supporting exclusion by index would be ideal.

@27Saumya
Copy link
Contributor Author

I think it would make sense to also support lists of Item objects into the exclusions param.

@27Saumya can you please implement this suggestion? Being able to pass the actual Item objects to exclude rather than only supporting exclusion by index would be ideal.

@krittick how that though like what shall I typehint?
This: exclusions: List[Union[discord.ui.Button, discord.ui.Select]]
?

@plun1331
Copy link
Member

I think it would make sense to also support lists of Item objects into the exclusions param.

@27Saumya can you please implement this suggestion? Being able to pass the actual Item objects to exclude rather than only supporting exclusion by index would be ideal.

@krittick how that though like what shall I typehint? This: exclusions: List[Union[discord.ui.Button, discord.ui.Select]] ?

it should be a list of discord.ui.Item iirc

@krittick
Copy link
Contributor

krittick commented Apr 11, 2022

List[Union[int, discord.ui.Item]]

Then just use isinstance to check what's being passed and handle appropriately.

@Dorukyum
Copy link
Member

Perhaps it should only allow item types that can be disabled.

auto-merge was automatically disabled April 12, 2022 08:38

Head branch was pushed to by a user without write access

@27Saumya 27Saumya requested a review from Lulalaby April 12, 2022 16:23
@Lulalaby Lulalaby enabled auto-merge (squash) April 12, 2022 16:43
Copy link
Member

@plun1331 plun1331 left a comment

Choose a reason for hiding this comment

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

This is only disabling the things passed to the exclusions parameter, the exact opposite of what is wanted.

@UP929312
Copy link
Contributor

This is only disabling the things passed to the exclusions parameter, the exact opposite of what is wanted.

Was going to say, what's the point in the current script? It just disables all those in the list.
Surely you want to disable everything not in the list?

discord/ui/view.py Outdated Show resolved Hide resolved
Co-authored-by: plun1331 <49261529+plun1331@users.noreply.github.com>
@Lulalaby Lulalaby enabled auto-merge (squash) April 21, 2022 18:52
Lulalaby
Lulalaby previously approved these changes Apr 24, 2022
plun1331
plun1331 previously approved these changes Apr 25, 2022
@Lulalaby Lulalaby requested review from Dorukyum and removed request for krittick and Middledot April 25, 2022 18:07
@27Saumya 27Saumya requested review from Middledot and krittick April 26, 2022 08:57
auto-merge was automatically disabled April 26, 2022 09:01

Head branch was pushed to by a user without write access

@27Saumya 27Saumya dismissed stale reviews from plun1331 and Lulalaby via 667a9e1 April 26, 2022 09:01
@27Saumya 27Saumya requested a review from Lulalaby April 26, 2022 09:04
@Lulalaby Lulalaby enabled auto-merge (squash) April 26, 2022 12:42
Lulalaby
Lulalaby previously approved these changes Apr 26, 2022
discord/ui/view.py Outdated Show resolved Hide resolved
Copy link
Member

@plun1331 plun1331 left a comment

Choose a reason for hiding this comment

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

This change should finish up the PR

discord/ui/view.py Outdated Show resolved Hide resolved
Co-authored-by: plun1331 <49261529+plun1331@users.noreply.github.com>
@plun1331 plun1331 added status: awaiting review Awaiting review from a maintainer feature Implements a feature labels Apr 29, 2022
@Lulalaby Lulalaby merged commit fe5803f into Pycord-Development:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Implements a feature status: awaiting review Awaiting review from a maintainer
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants