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

Sorting #191

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Sorting #191

wants to merge 5 commits into from

Conversation

GuiMF
Copy link

@GuiMF GuiMF commented Mar 12, 2025

Created a button to sort countries and communities per capita.
This was my first time working with Svelte, I've had experience with js and next.js, tailwind etc etc, but my code isn't the most elegant. The button doesn't resize on mobile properly, not catastrophic, and it is an easy fix I think, I simply don't know the codebase well enough (I hate CSS).

Copy link

netlify bot commented Mar 12, 2025

👷 Deploy request for btcmap pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 516a0c2

@GuiMF GuiMF closed this Mar 12, 2025
@GuiMF GuiMF reopened this Mar 12, 2025
@escapedcat
Copy link
Contributor

Thanks!
Also thanks for you comments inside your code to explain your reasoning. You could switch those from the code to the PR comments to avoid letting them get merged.

  • Have you tried to optimize your code asking AI for help? I'm also new to svelte and it helps to ask i.e. "This is how I do stuff in react, how would I do it in svelte?"
  • Not sure if a whole extra button is needed. Can you try to avoid it? Looks like you copied an existing component to add you're functionality? This should be separated if possible.
  • Would you mind adding screenshots of your feature to the PR?
  • With CSS we can help, no worries.

@GuiMF
Copy link
Author

GuiMF commented Mar 12, 2025

The Button I created was unnecessary, I just used the primary button and added a function to the click. I don't know if the way I imported the function was right but it works, I tried to add it to the utils but it wasn't working... Also removed comments... I'll add screenshots once I have better wifi, the stats seem to not be loading.

@GuiMF
Copy link
Author

GuiMF commented Mar 12, 2025

Screenshot 2025-03-12 at 17 19 33 This is what sorting by GDP looks like. Pressing the "Toggle Sort" toggles it back to sorting by total locations. It still displays the locations by GDP.

@dadofsambonzuki
Copy link
Member

I think we can just remove the button and have sort up/down arrows on each relevant column.

People often want to order by absolute number too.

Position column should still represent the default sort, which is a weighted number based on up-to-datedness.

We also need to handle the case where there is no population tag present.

We should also apply this to the country pages.

@GuiMF
Copy link
Author

GuiMF commented Mar 12, 2025

That does sound like a good idea to make the UI simpler, I used a button because I didn't know how to create the arrows.

As for the grade problem that's a bit of an issue because the grade is simply given to the top 3 objects in the list, with that said the grade would have to be a property given to the object itself and not the position of the leader board.

The no population tag issue is something I completely forgot about... I'll take a look!

This has already been implemented on the country pages.

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.

3 participants