Skip to content

Conversation

etroo44
Copy link
Contributor

@etroo44 etroo44 commented Oct 22, 2022

Hello, I have successfully added the select all toggle to the tracks table.

Closes #252

@NeoCoderMatrix86 NeoCoderMatrix86 self-requested a review October 28, 2022 09:11
Copy link
Owner

@NeoCoderMatrix86 NeoCoderMatrix86 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 your contribution 👍 . I did some checks and would like you to change some minor stuff:

  1. The "Select All" checkbox doesn't react to a track beeing unselected. As a user I would expect a state like "partly selected":
    grafik

  2. The code can be shortened:
    <Check TValue="bool" CheckedChanged="SelectAllTracks" Checked="selectedTracks.Any()" />

void SelectAllTracks(bool select)
    {
        if(select)
        {
            selectedTracks.AddRange(Cuesheet.Tracks);
        }
        else
        {
            selectedTracks.Clear();
        }
    }
  1. The method "SelectAllTracks" should be starting with "On", for example "OnSelectAllTracksChanged".

If you could edit these things, I will include the pull request. I would be happy to see more contributions from you, so if you want, have a look around the issues ;).

@etroo44
Copy link
Contributor Author

etroo44 commented Oct 31, 2022

Sure no problem, I'll fix it and create a new pull request.

@etroo44
Copy link
Contributor Author

etroo44 commented Nov 5, 2022

Hey, I changed the Select All functionality as you wanted and fixed some bugs, you forgot to remove a track from the selectedTracks list when removing a track from Cuesheet.Tracks in some places.

Copy link
Owner

@NeoCoderMatrix86 NeoCoderMatrix86 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 your contribution and the fixes in code! I really appriciate this :).

What I am missing is the feature of the new select all, that not all tracks are selected. I wasn't clear I see, so I have a screenshot here from blazorise examples:

grafik

The example can be found here: https://bootstrap5demo.blazorise.com/tests/datagrid

This should be included and then the feature is done :).

@etroo44
Copy link
Contributor Author

etroo44 commented Nov 8, 2022

Okay, no problem. I hope everything is now as you wanted.

@NeoCoderMatrix86
Copy link
Owner

Thanks for your contribution, now everything works fine. Also thanks for fixing bugs, I'll look forward for more contributions from you :).

@NeoCoderMatrix86 NeoCoderMatrix86 merged commit 1859e26 into NeoCoderMatrix86:master Nov 9, 2022
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.

Add "Select All" to tracks table

2 participants