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.

Sign up for GitHub

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

Jump to bottom

feat: add select all button to delete feed #234

Merged
merged 6 commits into from
Mar 14, 2024
Merged

feat: add select all button to delete feed #234

merged 6 commits into from
Mar 14, 2024

Conversation

dracarys18
Copy link
Contributor

  • Added a Select All button in deleted feed screen
  • Formatting

spacecowboy
spacecowboy requested changes Mar 13, 2024
View reviewed changes
Copy link
Owner

@spacecowboy spacecowboy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It looks pretty good.

I've noted some comments and suggestions.

app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
@spacecowboy
Copy link
Owner

You could keep it simple (because it's tricky) and just make the select-all button be dumb: it always selects all.

@dracarys18
Copy link
Contributor Author

You could keep it simple (because it's tricky) and just make the select-all button be dumb: it always selects all.

I would prefer it to be a checkbox with deselect. Because let's say I selected all of the entries with Select All button and now I have to deselect them then I have to do it one by one which is not really convenient. What are your thoughts?

@spacecowboy
Copy link
Owner

I would prefer it to be a checkbox with deselect. Because let's say I selected all of the entries with Select All button and now I have to deselect them then I have to do it one by one which is not really convenient. What are your thoughts?

Sure, I don't mind. But it's tricky as I said.

spacecowboy
spacecowboy requested changes Mar 13, 2024
View reviewed changes
Copy link
Owner

@spacecowboy spacecowboy left a comment

Choose a reason for hiding this comment

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

Some additional changes after adding the derivedState mentioned previously

app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nononsenseapps/feeder/ui/compose/deletefeed/DeleteFeedScreen.kt Outdated Show resolved Hide resolved
@dracarys18
Copy link
Contributor Author

Some additional changes after adding the derivedState mentioned previously

I have resolved them please check

@spacecowboy spacecowboy merged commit 10437fa into spacecowboy:master Mar 14, 2024
1 check passed
@spacecowboy
Copy link
Owner

Looks good now @dracarys18 Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacecowboy spacecowboy spacecowboy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dracarys18 @spacecowboy