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

Debugging name sort issue #973

Merged
merged 5 commits into from
Apr 26, 2021
Merged

Debugging name sort issue #973

merged 5 commits into from
Apr 26, 2021

Conversation

RyRy79261
Copy link
Contributor

closes #970

@render
Copy link

render bot commented Apr 23, 2021

Your Render PR Server URL is https://files-ui-dev-pr-973.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c21cmars1ghdsjqomv30.

@render
Copy link

render bot commented Apr 23, 2021

Your Render PR Server URL is https://files-landing-dev-pr-973.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c21cmb3s1ghdsjqon040.

@render
Copy link

render bot commented Apr 23, 2021

Your Render PR Server URL is https://chainsafex-dev-pr-973.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c21cmbjs1ghdsjqon0f0.

@RyRy79261 RyRy79261 marked this pull request as ready for review April 26, 2021 11:50
FSM1
FSM1 suggested changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

the husky.sh file should be removed, otherwise looks good

@RyRy79261 RyRy79261 requested a review from FSM1 April 26, 2021 12:37
@Tbaut Tbaut added the Status: Review Needed 👀 Added to PRs when they need more review label Apr 26, 2021
Tbaut
Tbaut requested changes Apr 26, 2021
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have the following when sorted alphabetically:

  • logo-512.png
  • logo.png

although I'd expect the logo.png to be first. I believe it's because of the ignore punctiation flag, that puts the "5" before the p from "png"

packages/files-ui/src/Components/Modules/FileBrowsers/views/FilesTable.view.tsx Outdated Show resolved Hide resolved
@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. and removed Status: Review Needed 👀 Added to PRs when they need more review labels Apr 26, 2021
@RyRy79261
Copy link
Contributor Author

@Tbaut So I ran a test with and without the punctuation thing, I think its because the character at [4] is a dash vs a . so not sure how those are prioritized, if its 5 vs j it would be first.

Maybe trimming extensions?

@RyRy79261 RyRy79261 requested a review from Tbaut April 26, 2021 16:45
@Tbaut
Copy link
Collaborator

Tbaut commented Apr 26, 2021

Maybe trimming extensions?

I'm just not sure why we want to ignore punctuation?

@RyRy79261
Copy link
Contributor Author

@Tbaut I've removed ignore punctuation config

@Tbaut
Copy link
Collaborator

Tbaut commented Apr 26, 2021

yeah sorry, I saw it after commenting :embarrassed:

@Tbaut Tbaut added Status: Merge Ready ✅ and removed Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. labels Apr 26, 2021
@RyRy79261 RyRy79261 merged commit 056889d into dev Apr 26, 2021
@RyRy79261 RyRy79261 deleted the bug/name-sorting-970 branch April 26, 2021 17:21
@FSM1 FSM1 mentioned this pull request Apr 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tbaut Tbaut Tbaut approved these changes

@FSM1 FSM1 FSM1 approved these changes

@tanmoyAtb tanmoyAtb Awaiting requested review from tanmoyAtb

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

Successfully merging this pull request may close these issues.

File Browser Name sorting is broken
3 participants
@RyRy79261 @Tbaut @FSM1