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

MDE/PKFE-19 #49

Merged
merged 15 commits into from
Sep 1, 2024
Merged

MDE/PKFE-19 #49

merged 15 commits into from
Sep 1, 2024

Conversation

@mantvydasdeltuva mantvydasdeltuva added the enhancement New feature or request label Aug 28, 2024
@mantvydasdeltuva mantvydasdeltuva self-assigned this Aug 28, 2024
Import...
</MenuItem>,
<Divider key='divider-import' />
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

if(item.fileType === undefined) and if(item.fileType === FileTypes.FOLDER || item.fileType === undefined) statements seem to do the same thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed with new logic


if (input.length > 50) {
return 'Input must be less than 50 characters';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also validate for forbidden file name characters or forbidden combinations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added checks:

  • contains null, /, *, ?, [, ]
  • cannot be . or ..
  • cannot start or end with .

Copy link
Collaborator

Choose a reason for hiding this comment

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

It will work for now, but more character checks should be added in the future

Copy link
Collaborator Author

@mantvydasdeltuva mantvydasdeltuva Sep 1, 2024

Choose a reason for hiding this comment

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

Created new story in backlog: PKFE-43

Copy link
Collaborator

@justinnas justinnas left a comment

Choose a reason for hiding this comment

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

Some problems mentioned in comments on code.
In addition, I don't think a user should be able to change the file type while naming or renaming, or the file type should be seperate from the file name with maybe a dropdown menu. Non-tech savy users could name the file wrong which would cause problems, especially when exporting.

if (input.length > 50) {
return 'Input must be less than 50 characters';
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to validate if a file with specified name already exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated workspaceContextProvider.tsx to store file hierarchy and it can be accessed anywhere. New function in utils/helpers.ts provides ability to search for specific file. Added check for existing file while renaming or creating new file. Also added functionality to specify files' extension and removed the ability to define your own extension.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work on the extensions! I just found one more small problem, it doesn't check for cases, I was able to create "A.csv" and "a.csv", it should be able to be solved by temporarily converting file names to lower-cases before checking if they exist.

Copy link
Collaborator Author

@mantvydasdeltuva mantvydasdeltuva Sep 1, 2024

Choose a reason for hiding this comment

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

Updated helpers.tsx: doesFileExist function. Fixed textfield 'jumping' during error.

}

if (/[*?[\]]/.test(input)) {
return 'Input contains forbidden glob characters (*, ?, [, ])';
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove the 'glob' word because I don't think many people would know it's meaning, "...forbidden characters..." should be enough

Copy link
Collaborator Author

@mantvydasdeltuva mantvydasdeltuva Sep 1, 2024

Choose a reason for hiding this comment

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

Removed!

Copy link
Collaborator

@justinnas justinnas left a comment

Choose a reason for hiding this comment

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

Amazing work!!

@mantvydasdeltuva mantvydasdeltuva merged commit 7f6ff19 into main Sep 1, 2024
0 of 3 checks passed
@mantvydasdeltuva mantvydasdeltuva deleted the MDE/PKFE-19 branch September 1, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants