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

parse failed message added in common import container #1823

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

Yagnik56
Copy link
Collaborator

@Yagnik56 Yagnik56 commented Aug 9, 2024

This PR contains the code change of adding a parse fail error message in a common import container.
(JSON is used to example only)
image

@Yagnik56 Yagnik56 requested review from zackcl and danoswaltCL August 9, 2024 08:45
@Yagnik56 Yagnik56 self-assigned this Aug 9, 2024
@Yagnik56 Yagnik56 linked an issue Aug 9, 2024 that may be closed by this pull request
@Yagnik56 Yagnik56 linked an issue Aug 12, 2024 that may be closed by this pull request
@Yagnik56 Yagnik56 merged commit a398be3 into dev Aug 14, 2024
8 checks passed
@Yagnik56 Yagnik56 deleted the enhancement/parse-fail-error-msg-in-import-modal branch August 14, 2024 14:28
@zackcl
Copy link
Collaborator

zackcl commented Aug 14, 2024

@Yagnik56 Just to clarify, can the "Invalid JSON" error message show up when importing an invalid JSON file? Shouldn't it be immediately converted to the compatibility table (with a single "Incompatible" row) if the JSON file is invalid when importing a feature flag?

I'm asking because my PR (#1824) will remove the code changes you made with this PR (I had to resolve the merge conflicts). In my PR, it only shows the invalid CSV error message when the user uploads an invalid CSV file.

@Yagnik56
Copy link
Collaborator Author

Yagnik56 commented Aug 14, 2024

@zackcl we have to pass observable (optional) as true to show that invalid message and it is dynamic so it will read the file type and show there. I just used JSON as an example, it works the same way for CSV. For JSON we won't pass observable value so it won't show that error. I know we don't want to show it for JSON but it will be nice to keep it dynamic with filetype we are choosing I feel.

@zackcl
Copy link
Collaborator

zackcl commented Aug 14, 2024

I'm not sure what you mean by "pass observable (optional) as true" but my PR basically does the same thing as what you did to show the invalid error message when the input variable importFailed is set to true.

    <span *ngIf="importFailed" class="ft-12-400 import-failed-msg">
      {{ 'feature-flags.upsert-list-modal.import-csv.error.message.text' | translate }}
    </span>

I just wanted to clarify if removing your code changes won't affect your other code for importing JSON file, and it seems it doesn't affect it since we never show it for JSON. Thanks!

@Yagnik56
Copy link
Collaborator Author

Rather than putting hard coded messages keep it dynamic for fileType, it will be better in my view. @zackcl

@zackcl
Copy link
Collaborator

zackcl commented Aug 14, 2024

I think it's fine to keep it static for now since it's never used for JSON. Also, the message is actually fine to be used for both types:

"feature-flags.upsert-list-modal.import-csv.error.message.text": "Import failed. Please check your file and try again.",

Also, I think we want to maintain such messages in the en.json file for best practice. If you have any specific concerns about the changes that will be made with my PR regarding this, please feel free to leave comments there (#1824). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FF: Import Modal Import Fail state FF: Design needed for Import Modal: Failed to parse file.
3 participants