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: allow third click to disable sorting (fixes #132) #139

Merged
merged 6 commits into from
Oct 26, 2024
Merged

feat: allow third click to disable sorting (fixes #132) #139

merged 6 commits into from
Oct 26, 2024

Conversation

Atharva-Kanherkar
Copy link
Contributor

Resolves #132

Please see the video below :

Ubuiquity_DAOissue1.webm

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 24, 2024 •
edited
Loading

@zugdev
Copy link
Collaborator

zugdev commented Oct 25, 2024 •
edited
Loading

Hey, the none option is working but you have to handle when user clicks on another sorting handle as well. Sorting is correct in that case but it should not display the asc/desc indicator

image

@zugdev
Copy link
Collaborator

zugdev commented Oct 25, 2024 •
edited
Loading

These lines previously did that by clearing the data-sorted attribute for all other options

work.ubq.fi/src/home/sorting/sorting-manager.ts

Lines 139 to 143 in cb5e4ad

input.parentElement?.childNodes.forEach((node) => {
if (node instanceof HTMLInputElement) {
node.setAttribute("data-ordering", "");
}
});

@zugdev
Copy link
Collaborator

zugdev commented Oct 25, 2024 •
edited
Loading

Also the current sorting CSS looks weird, @0x4007. The gray background toggle seems binary, we should either have it be gray when it's not "none" or not have this gray background ever.

image

image

@zugdev
Copy link
Collaborator

zugdev commented Oct 25, 2024

The rest of it looks good to me @Atharva-Kanherkar, just please make sure you clear all other options states on click.

@Atharva-Kanherkar
Copy link
Contributor Author

These lines previously did that by clearing the data-sorted attribute for all other options

work.ubq.fi/src/home/sorting/sorting-manager.ts

Lines 139 to 143 in cb5e4ad

input.parentElement?.childNodes.forEach((node) => {
if (node instanceof HTMLInputElement) {
node.setAttribute("data-ordering", "");
}
});

Oh okay! I might have missed that, thank you for mentioning it. I'll review and commit some changes!

@Atharva-Kanherkar
Copy link
Contributor Author

Also the current sorting CSS looks weird, @0x4007. The gray background toggle seems binary, we should either have it be gray when it's not "none" or not have this gray background ever.

image

image

So you want me to fix this in this PR? or this is just intended as a discussion?

@Atharva-Kanherkar
Copy link
Contributor Author

Hey @zugdev I have added the changes!:) Now you can see it works as expected <3

@zugdev
Copy link
Collaborator

zugdev commented Oct 25, 2024

Looks good to me, just wait for code owner to review

@Atharva-Kanherkar
Copy link
Contributor Author

@0x4007 Even after optimizing the code, The tests fail, is there a possibility that we can increase the timeouts in cypress?

@zugdev
Copy link
Collaborator

zugdev commented Oct 25, 2024 •
edited
Loading

@0x4007 Even after optimizing the code, The tests fail, is there a possibility that we can increase the timeouts in cypress?

Cypress is broken and up to be fixed at #109, if you want to fix them... But don't worry much for this PR I guess.

0x4007
0x4007 reviewed Oct 25, 2024
View reviewed changes
src/home/sorting/sorting-manager.ts Outdated
observer.observe(issuesContainer, { childList: true });

// Add event listener for input changes to filter and update URL
// Debounce input to avoid frequent filtering
Copy link
Member

Choose a reason for hiding this comment

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

Why do you debounce the input? I don't think this is normally a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

he was trying to make cypress work haha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to optimize some code because I thought the tests were failing because of me. I will remove the logic ASAP

0x4007
0x4007 reviewed Oct 25, 2024
View reviewed changes
yarn.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should regenerate your lock file. I don't see why it should have changed in this pull.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I still dont understand why it happened. I will change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regenerating yarn.lock, but still appears in the pull, what can be the reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a problem with yarn on itself yarnpkg/yarn#4379

0x4007
0x4007 reviewed Oct 25, 2024
View reviewed changes
src/home/sorting/sorting-manager.ts Outdated
@@ -9,6 +9,8 @@ export class SortingManager {
private _filterTextBox: HTMLInputElement;
private _sortingButtons: HTMLElement;
private _instanceId: string;
private _sortingState: { [key: string]: "unsorted" | "ascending" | "descending" } = {}; // Track state for each sorting option
private _filterTimeout: number | null = null; // Timeout ID for debouncing
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove the debounce logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, ill remove it

@0x4007
Copy link
Member

0x4007 commented Oct 25, 2024

Feels nice though I just tested it. I think when you address my comments I can merge!

@Atharva-Kanherkar
Copy link
Contributor Author

@zugdev @0x4007 Any suggestions on how can I remove the yarn.lock from the pull ? Is this mergeable?

@zugdev
Copy link
Collaborator

zugdev commented Oct 26, 2024

@zugdev @0x4007 Any suggestions on how can I remove the yarn.lock from the pull ? Is this mergeable?

You don't have to remove yarn.lock from the pull, just make sure you ran yarn and commited the appropriate yarn.lock file. You haven't installed any new packages and should be good to go

@Atharva-Kanherkar
Copy link
Contributor Author

@zugdev @0x4007 Any suggestions on how can I remove the yarn.lock from the pull ? Is this mergeable?

You don't have to remove yarn.lock from the pull, just make sure you ran yarn and commited the appropriate yarn.lock file. You haven't installed any new packages and should be good to go

So acc to u it seems mergeable?

@0x4007 0x4007 merged commit 36d04bb into ubiquity:development Oct 26, 2024
1 of 2 checks passed
@ubiquity-os ubiquity-os bot mentioned this pull request Oct 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zugdev zugdev zugdev left review comments

@0x4007 0x4007 0x4007 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.

Allow a third click to disable sorting
3 participants
@Atharva-Kanherkar @zugdev @0x4007