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

preview for html and csv added #175

Closed
wants to merge 5 commits into from
Closed

Conversation

BrawlerXull
Copy link
Contributor

Screenshot 2024-02-24 at 3 11 29 AM Screenshot 2024-02-24 at 3 14 14 AM

Added preview for CSV and HTML format
Fixes #155 #156

@BrawlerXull
Copy link
Contributor Author

@ashitaprasad please review the changes

@animator
Copy link
Member

@BrawlerXull Add appropriate test cases for the changes that you have made in the test folder.

@BrawlerXull
Copy link
Contributor Author

BrawlerXull commented Feb 23, 2024

@BrawlerXull Add appropriate test cases for the changes that you have made in the test folder.

Working on it :)
Rest everything in the code is fine? Any suggestions?

@BrawlerXull
Copy link
Contributor Author

BrawlerXull commented Feb 23, 2024

Screenshot 2024-02-24 at 3 32 37 AM

I've added tests for the code @animator is it ok?

}

void processCsv(String body) {
print("hello");
Copy link
Member

Choose a reason for hiding this comment

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

Why is print("hello") here?

@override
void initState() {
super.initState();
csvData = [];
Copy link
Member

Choose a reason for hiding this comment

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

Why is csvData state being maintained in Previewer Widget.
Everything related to csv shall happen in a separate widget and the purpose of this widget is to call the right widget based on mimetype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i create a new widget for CsvPreviewer and move the logic there?

Copy link
Contributor Author

@BrawlerXull BrawlerXull Feb 23, 2024

Choose a reason for hiding this comment

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

I am really sorry for the silly mistakes I'll take care of writting clean code from now onwards

Copy link
Member

Choose a reason for hiding this comment

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

yes it should be a new widget.
Also, CSV & HTML previewers should be in separate PRs.
Not the same PR.

@animator
Copy link
Member

For text/html make a separate PR. Also,

Expected Response:

Screenshot 2024-02-23 at 7 12 45 AM

This PR:
Screenshot 2024-02-24 at 6 39 31 AM

@animator
Copy link
Member

animator commented Mar 8, 2024

Closing this PR as CSV was merged. You can raise a separate PR for html.

@animator animator closed this Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add response previewer for text/html
3 participants