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

Resolve Issue #582 #692

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

Conversation

Maria Paola added 3 commits February 12, 2025 16:23
* main: (112 commits)
  changing field order
  adapting field changes
  changing field order
  adapting field changes
  Use up-to-date reuse tool in GitHub action
  Convert dep5 file to REUSE.toml
  fix pavement select option
  Remove disabled dataset filter button
  fixing ShieldingPostInFrontOfStation
  Improve map view performance by rendering OverviewCardItem elements only if map is not visible
  Update package-lock.json
  Increase z-index for active layer and marker
  Open record detail from map view in same tab
  Use pinia store to improve map-view implementation
  Implement URL state for map view
  Fix bug where map-view records data was not loaded when map-view was closed and reopened
  Style close-button for map view dialog
  Increase limit of records that are loaded from timeseries API
  Fix bug where an undefined value was accessed
  Remove memoize from meta data fetching and configure stale time for query instead
  ...

# Conflicts:
#	databrowser/src/pages/datasets/overview/OverviewListPage.vue
Copy link
Collaborator

@gappc gappc left a comment

Choose a reason for hiding this comment

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

Just some minor things to fixd, thx 👍

@RudiThoeni
Copy link
Member

is this PR ready to merge?

@mariapaolasmnt
Copy link
Author

is this PR ready to merge?

yes, the PR is ready to merge

@sseppi
Copy link
Contributor

sseppi commented Feb 20, 2025

@gappc can you please do the final check and notify @RudiThoeni when you are done?

@gappc
Copy link
Collaborator

gappc commented Feb 20, 2025

@sseppi some issues need to be resolved, otherwise we introduce bugs with this PR. @mariapaolasmnt please take a look at the latest comments, specially about the filters

Comment on lines +129 to +131
tableFilters.value = tableFiltersSelected.map(filter => ({
...filter
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still not sure what you're trying to accomplish here. In effect, what this code does is, that it generates a new pointer on a new array of objects, where each object gets a new pointer to its contents. Is this really necessary here? Because in that case, maybe a structuredClone would be more appropriate and easier to understand.

If it's not necessary, I would just assign tableFiltersSelected to tableFilters.value.

Or am I missing something?

Comment on lines +160 to +161
tableFilters.some((filter) =>
['isnull', 'isnotnull'].includes(filter.operator)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this works only for datasets from the content API (previously named tourism API), because the filters isnull and isnotnull exist only in the content API

Please take a look at

@@ -61,7 +61,7 @@ export const useTableFilterStore = defineStore('tableFilterStore', () => {
if (propertyPath != null) {
tableFilters.value = [
...tableFilters.value,
{ propertyPath, title, operator: 'eq', value: '' },
{ propertyPath, title, operator: 'like', value: '' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this works only for datasets from the content API (previously named tourism API), because the filter like exists only in the content API

Please take a look at

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.

4 participants