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

Move and Store the Data Source and Data Store Logos on Polypheny-DB #485

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

malikrafsan
Copy link

@malikrafsan malikrafsan commented Apr 2, 2024

Summary

In this pull request, we move the logos from stored at the Frontend (Polypheny-UI) to the Backend (Polypheny-DB). We also add the logo path/icon identifier on the response regarding Data Store and Data Source (/getSnapshot). By doing this, we can simplify the process of adding new Data Stores / Data Sources by storing the logos in Polypheny-DB sources together with the adapter implementation.

Fixes: #222

Changes

  • Add adapterLogo attribute on AdapterProperties annotation interface
  • Send adapterLogo attribute with existing adapter information
  • Change Polypheny-UI logic regarding adapterLogo

Related PRs


Screenshots

  • Asset files can be served by Polypheny-DB, under /public hosted path
    Screenshot from 2024-04-05 00-59-49

  • Polypheny-UI can access the Polypheny-DB static files and the Data Source and Data Store logos are shown successfully
    Screenshot from 2024-04-05 01-02-00

@malikrafsan malikrafsan changed the title feat: add adapter logo on each adapters Move and Store the Data Source and Data Store Logos on Polypheny-DB Apr 2, 2024
@malikrafsan malikrafsan marked this pull request as ready for review April 4, 2024 18:08
@hennlo hennlo requested review from datomo and vogti April 5, 2024 12:39
@hennlo hennlo added C-enhancement Category: An issue proposing an enhancement A-ui Area: UI S-in-progress Still working on this pull request labels Apr 5, 2024
@datomo
Copy link
Member

datomo commented Apr 8, 2024

@malikrafsan thank you very much for this PR, this looks really promising!
While I'm satisfied with most of it I have one bigger change request before the review:

At the moment you attach the logo to the AdapterTemplateModel, which is a part of the SnapshotModel.
The SnapshotModel is broadcast to the client on every update of the schema and therefore it includes as little data as possible.
With your approach the size is increased drastically, which is not ideal.
I would suggest that you expose the images via static routes from the backend and only attach the paths in the AdapterTemplateModel to circumvent that.

@datomo datomo marked this pull request as draft April 8, 2024 07:47
@malikrafsan
Copy link
Author

Hi @datomo, thank you so much for the feedback and for taking the time to review the PR. I really appreciate your insights and suggestions!

I'd like to clarify that I have indeed followed the suggested approach. The images are served through static routes from the backend (<origin>/public/assets/dbms-logos/<filename>), and only the paths to these images (/assets/dbms-logos/<filename>) are linked in the AdapterTemplateModel. There's no direct embedding of the logo images (base64 image string) within the AdapterTemplateModel

However, I am afraid that my understanding is incorrect or that there are any aspects of the implementation that need further clarification. Please kindly let me know if so. Your guidance is greatly appreciated!

Thank you once again for your time and consideration

Screenshot from 2024-04-09 22-36-08

Screenshot from 2024-04-09 22-36-46

@datomo
Copy link
Member

datomo commented Apr 9, 2024

Hey @malikrafsan
My bad, I totally missed this. You are right, then you already used the optimal way.

@malikrafsan malikrafsan marked this pull request as ready for review April 9, 2024 17:21
@malikrafsan
Copy link
Author

Thanks for confirming @datomo ! It's great to know this is the optimal approach. I'll mark this PR as ready for review then, looking forward to your review! Once again, thank you so much

Copy link
Member

@datomo datomo left a comment

Choose a reason for hiding this comment

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

Thank you @malikrafsan, this looks really good and is appreciated.

The resource location is one of the main point, which requires some change, else this is fine by me.

Copy link
Member

Choose a reason for hiding this comment

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

Could you move the logos to each plugin directly?
When they have to be included in the core modules, it removes the separation between plugins and core.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean move the logo file? It seems like it would be a bit complicated as currently in this MR, we serve the logo files if the files are inside public directory from resources

@malikrafsan malikrafsan requested a review from datomo April 24, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ui Area: UI C-enhancement Category: An issue proposing an enhancement S-in-progress Still working on this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants