Feature/quickview

[MatteoBiasi]

No description provided.

[gappc]

Hi @MatteoBiasi, thank you for this PR. Please note that it has conflicts with the current development branch, it can not be merged in its current state.

Please consider doing either a rebase + force push (should be fine, I don't think that there are other people that depend on this branch) or merge the current development branch into this one and fix the problems.

Let me know when you think the branch is ready and I'll start a code review.

Some things I noted while a took a first look:

• use <script setup lang="ts"> in all components
• use correct TypeScript declarations in defineProps (see https://vuejs.org/guide/typescript/composition-api.html#typescript-with-composition-api)
• remove commented code

[MatteoBiasi]

@gappc i think it's better to leave this opened until our next meeting together for the following reasons:

1. I'm not confident to risolve conflicts in package.json.
2. All new components use <script setup lang="ts"> and defineProps looks to me properly used. So I need better to understand what do you mean.
3. The commented code is there because it appeared that the ComponentRenderer class resolves only components from a web-component library that has been already published, therefore without any explanation of that process we currently and momentarily had to implement a custom resolver.

[gappc]

@MatteoBiasi that's fine with me, see you in the next meeting 👍

[gappc]

Please load the OpenLayersMap component on-demand where you need eit (e.g. MapBase.vue). That way, the JavaScript for OpenLayers is loaded by the browser only if needed, e.g. it is not needed for the home page to show up.

See https://github.com/noi-techpark/it.bz.opendatahub.databrowser/blob/development/databrowser/src/domain/datasets/editView/EditToolBox.vue#L43 for an example

[MatteoBiasi]

Hi @gappc we fixed the majority of the optimizations requested with the code review. There's still something left out (see comments on specific conversations) and the conversations which have been left opened. With this ones we prefer first to produce the final coding documentation and then to proceed with the refactoring, in order to avoid to do things twice.

[gappc]

@MatteoBiasi I see you made some good progress, but there are still issues left, like usage of any type where concrete types would be more appropriate, usage of hand-crafted date/time manipulation and so on. In my opinion, things like these are simply best-practice and could be fixed already,

Either way, lets discuss these topics on our next meeting and do another iteration, I think were moving forward in the right direction.

[gappc]

@MatteoBiasi: there were new commits on the development branch that add a new dependency to the Data Browser packages (see https://github.com/noi-techpark/it.bz.opendatahub.databrowser/blob/b7013703f4c5986e9c86d82b5dca15742233fc1a/databrowser/package.json#L60).

Please merge / rebase that branch such that it contains the change, thank you :)

[MatteoBiasi]

Hi @gappc, since when I've merged the last release, I can no more load any view: QuickView, Detail View, Raw View etc. They're all empty. The issue seems related to data fetching, where the request never resolves. By checking the console, I see the following error:

queryObserver.js:331 TypeError: filterQuery?.replace is not a function
at parseFilterQuery (utils.ts:2:41)

But even if I change the parseFilterQuery function to make it not to fail, the API still doesn't return any data. The .env matches the example one. What should I do?

[gappc]

Hi @MatteoBiasi, could you please provide a branch + your env variables (env please via email as they contain possibly sensititve content)? Otherwise it's really hard to find out what's going on.

[gappc]

Hi @MatteoBiasi, I saw that this branch can not be merged, there are some conflicts. If you want to merge this branch, please bring it up to date and fix the issues. As an alternative, we could discuss during the next meeting if it is better to adhere the code to the upcoming guidelines (#319) before we merge it. What is your opinion?

[MatteoBiasi]

Hi @MatteoBiasi, I saw that this branch can not be merged, there are some conflicts. If you want to merge this branch, please bring it up to date and fix the issues. As an alternative, we could discuss during the next meeting if it is better to adhere the code to the upcoming guidelines (#319) before we merge it. What is your opinion?

I think it's better to merge this one and then do another PR for the CSS refactoring, otherwise we'll keep this one opened for too long. Anyway I propose to discuss this together tomorrow.