Skip to content

Sorting Tool - #35

Closed
TechSpiritSS wants to merge 5 commits into
developertools-tech:mainfrom
TechSpiritSS:Sorting
Closed

Sorting Tool#35
TechSpiritSS wants to merge 5 commits into
developertools-tech:mainfrom
TechSpiritSS:Sorting

Conversation

@TechSpiritSS

Copy link
Copy Markdown
Contributor

Type of Pull Request

  • Bug fix
  • Enhancement

Related Issue #s or links (if any):
#21

Description of Changes

Everything is working great and tested except for one bug.
User is required to click sort twice in case of New Line separator to get the desired result. I'm not sure why this is happening but I wasn't able to resolve this bug.

@netlify

netlify Bot commented Oct 6, 2022

Copy link
Copy Markdown

Deploy Preview for developertools-tech ready!

Name Link
🔨 Latest commit 9bf2981
🔍 Latest deploy log https://app.netlify.com/sites/developertools-tech/deploys/6341457e186e0800092e18df
😎 Deploy Preview https://deploy-preview-35--developertools-tech.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@dlford

dlford commented Oct 7, 2022

Copy link
Copy Markdown
Member

@TechSpiritSS I'll look into that issue and get back to you, we can't merge as is unfortunately.

@TechSpiritSS

Copy link
Copy Markdown
Contributor Author

:') well, that's fine, if you know the reason for the bug do let me know as well.

Comment thread pages/sorting.tsx Outdated

@dlford dlford left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@TechSpiritSS Awesome! Code looks solid, just a few UI tweaks.

ksnip_20221008-101144

ksnip_20221008-101317

Also, I think it would add a lot of utility if we could add a field for the output separator so you could change a newline separated list to comma separated, etc. (This would also require a sort option of "keep order").

If that's too much to ask I can create a new issue for it.

@TechSpiritSS

TechSpiritSS commented Oct 8, 2022

Copy link
Copy Markdown
Contributor Author

I think it's better to change that into new issue as I might not be able to commit those changes as I am travelling for next two days.

@dlford

dlford commented Oct 8, 2022

Copy link
Copy Markdown
Member

I think it's better to change that into new issue as I might not be able to commit those changes as I am travelling for next two days.

No worries, just the UI fixes then please

@dlford

dlford commented Oct 19, 2022

Copy link
Copy Markdown
Member

@TechSpiritSS are you still wanting to work on this?

@TechSpiritSS

Copy link
Copy Markdown
Contributor Author

Yes, I'll work on this, got some college stuff to deal with this week.

@dlford

dlford commented Oct 19, 2022

Copy link
Copy Markdown
Member

No worries, thank you!

@dlford dlford closed this Sep 1, 2023
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.

2 participants