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

Feature: Upload locally generated report to bowtie UI #297

Merged

Conversation

AgniveshChaubey
Copy link
Collaborator

This PR enables users to upload locally generated JSON data to the Bowtie UI. It allows for a more informative and comprehensive representation of their local reports.

@pre-commit-ci pre-commit-ci bot temporarily deployed to PyPI June 16, 2023 04:15 Inactive
@AgniveshChaubey AgniveshChaubey temporarily deployed to PyPI June 16, 2023 06:16 — with GitHub Actions Inactive
Copy link
Collaborator

@harrel56 harrel56 left a comment

Choose a reason for hiding this comment

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

I have one major design suggestion, which boils down to:

  • instead of using fetch() directly in component we could use loaders from react-router. As loaders can be parametrized we could have just one route for all the routes that require report data from web. https://reactrouter.com/en/main/route/loader

  • ideally, App component could be just a component for pure presentation (maybe this component should get a rename?) - by that I mean it won't make any HTTP request or any other things like that, but it will receive report data (lines) via prop or via beforementioned loader data

  • as for page where user is expected to upload local files - it could be implemented as a component that wraps App component, and it either renders the file drop area or the App with data extracted from provided file

I could use loaders from the beginning, but now I think it gets more relevant as we have 2 sources of data. Also, if loader API doesn't work for us we could provide out own abstraction for providing data from the web, which probably would be another version of wrapper for App.

Let me know what you think about this approach or if anything is unclear

@AgniveshChaubey
Copy link
Collaborator Author

AgniveshChaubey commented Jun 19, 2023

Was reading about loaders and yes, it does make sense to utilize the loader for loading the data. However, can you further explain what do you mean by -could have just one route for all the routes that require report data from web

Yes, It's better to fully utilize the feature of resuibility of components. Will create a wrapper component that will render either file drop area or theApp component based on the conditions.

@harrel56
Copy link
Collaborator

I had in mind that we could simplify routes configuration by having more generic routing, like:

createHashRouter([
  {
    path: "/:draftName",
    element: <App .../>,
    loader: ({ params }) => {
      return someFetchingFunction(params.draftName);
    },
  },
]);

@AgniveshChaubey
Copy link
Collaborator Author

tried this loader function -

    {
      path: "/:draftName",
      element: <ReportDataHandler />,
      loader: ({ params }) => {
        console.log(params.draftName);
        return fetch(
          `https://bowtie-json-schema.github.io/bowtie/${params.draftName}.jsonl`
        )
          .then((response) => response.text())
          .then((jsonl) => {
            const dataObjectsArray = jsonl.trim().split(/\n(?=\{)/);
            const lines = dataObjectsArray.map((line) => JSON.parse(line));
            return lines;
          });
      },
    },

Even though the draftName variable correctly reflects the draft name in the console, why is it resolved as undefined in the URL?
getting this error GET https://bowtie-json-schema.github.io/bowtie/undefined.jsonl 404

@harrel56
Copy link
Collaborator

Are you sure the request call doesn't come from somewhere else in the code? Because if it prints it right it should also send request to a valid URL. If you still won't be able to fix it, you could commit your changes and then I could have a look

@AgniveshChaubey
Copy link
Collaborator Author

Yes, the call was from another component! 😐

@AgniveshChaubey AgniveshChaubey temporarily deployed to PyPI June 20, 2023 04:37 — with GitHub Actions Inactive
@pre-commit-ci pre-commit-ci bot temporarily deployed to PyPI June 21, 2023 21:47 Inactive
@AgniveshChaubey
Copy link
Collaborator Author

AgniveshChaubey commented Jun 21, 2023

used Context APIs to make the theme data available centrally!
(in near future)-- Using content API, will also update the way data is fetched and transferred to different components to avoid prop drilling.

@AgniveshChaubey AgniveshChaubey temporarily deployed to PyPI June 22, 2023 03:54 — with GitHub Actions Inactive
frontend/src/ReportDataHandler.js Outdated Show resolved Hide resolved
frontend/src/components/DragAndDrop/DragAndDrop.js Outdated Show resolved Hide resolved
frontend/src/components/DragAndDrop/DragAndDrop.js Outdated Show resolved Hide resolved
frontend/src/components/DragAndDrop/DragAndDrop.js Outdated Show resolved Hide resolved
frontend/src/components/DragAndDrop/DragAndDrop.js Outdated Show resolved Hide resolved
frontend/src/context/ThemeContext.js Outdated Show resolved Hide resolved
frontend/src/context/ThemeContext.js Outdated Show resolved Hide resolved
@AgniveshChaubey AgniveshChaubey temporarily deployed to PyPI June 22, 2023 18:21 — with GitHub Actions Inactive
@AgniveshChaubey AgniveshChaubey temporarily deployed to PyPI June 22, 2023 20:05 — with GitHub Actions Inactive
@AgniveshChaubey
Copy link
Collaborator Author

Added the conditional rendering of components (<ReportData /> and <DragAndDrop />) based on the URL's hostname.

Most of the issues related to this specific PR have been resolved, and I think it is now ready to merge!

Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

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

One minor comment/question, after that feel free to merge!

@pre-commit-ci pre-commit-ci bot temporarily deployed to PyPI June 23, 2023 15:50 Inactive
@AgniveshChaubey AgniveshChaubey merged commit 964b0e7 into bowtie-json-schema:main Jun 23, 2023
18 checks passed
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.

None yet

3 participants