August 2024 UI and usability improvements #596

MatteoBiasi · 2024-09-04T14:59:52Z

This PR includes the the following issues:

As a Data Browser user I want that the Deprecated Fields are hidden per default also in the Table view, to not use deprecated values in my calls. #532
As a Data Browser user I want that the data coming from referenced calls are marked, to understand which calls I have to do to get the data I see. #533
As a Data Editor I want the possibility to add optional "Additional Properties" to a dataset (e.g. the eCharging accessability for the POI eCharging stations), to add specific information only with needed. #573
As a Data Consumer I want the side bar open per default and with more information about the dataset, to be warned about the options I have in the sidebar. #526
As a Data Consumer I want an easier way to access the data, to be quincker in find what I'm looking for #524
As a Data Consumer I want to easily access the Table view from the Dataset Overview page, to improve the navigation of the data. #527
As a Data Consumer I want in the Table View an easy way to change the dataset I looking, to be quicker in the navigation of the data. #525
As a Data Browser user I want a compacter table view, to have a better focus on data and key functionalities #575

sseppi · 2024-09-12T07:41:33Z

databrowser/src/config/builder/tourism/location.ts

@@ -185,6 +185,20 @@ export const locationTableCells = (): PropertyConfig[] => [
    class: 'w-52',
    objectMapping: { text: 'LocationInfo.RegionInfo.Name.{language}' },
  },
+
+  // FIXME: ADD FOR TESTING 533


@a-crea did you forget to remove the code you added for testing or shall we remove the comment // FIXME?

gappc · 2024-09-12T10:48:41Z

@MatteoBiasi @a-crea general remark for this PR: it is not up-to-date with the current development branch, please bring it up-to-date

gappc · 2024-09-12T12:27:45Z

I just noted that for this PR in TableView on mobile, the "Lines per page" - selectbox seems broken, see images below.

Note that the visualization on desktop seems odd, to.

PR:

Current development version (e.g. https://databrowser.opendatahub.testingmachine.eu/dataset/table/tourism/v1/Accommodation):

gappc · 2024-09-12T12:38:46Z

In the TableView, the new header has lots of white spaces (#575 is about a compacter TableView) and the select-bi / buttons / search input have different heights (small issue). See image below.

gappc · 2024-09-12T15:31:51Z

DetailView: referenced attributes are not visible at all, when the Load referenced attributes toggle is deactivated, see images below

Referenced attributes visible (correct)

Referenced attributes not visible at all (bug?)

gappc · 2024-09-12T15:33:36Z

Matter of taste: lots of white space in between Table and Toolbox, see image below

gappc · 2024-09-12T15:50:37Z

Proposal for referenced values: clicking on the reference link should open the data in a new tab, see image below.

gappc · 2024-09-12T16:01:15Z

DetailView for dataset with additional properties: the accordion arrow icon in the menu on the left is not visible on initial page load, see image below.

Example URL: BASE_URL/dataset/detail/tourism/v1/ODHActivityPoi/echarging_vek:hyc_150_19bz00171_goldrain:1#text-information

gappc · 2024-09-12T16:23:24Z

DetailView / EditView: opening the page of an additional property directly by inserting the URL into the browser opens the the page correctly, but the menu on the left is not updated correctly

Example link: BASE_URL//dataset/detail/tourism/v1/ODHActivityPoi/echarging_vek:hyc_150_19bz00171_goldrain:1#echargingdata

gappc · 2024-09-12T16:57:52Z

Strange things happen when the Toolbox in one of the Table/Detail/EditViews is open in desktop width (e.g. 1400px) and the browser window is resized to mobile widths (e.g. 600px):

In that case, nothing is visible anymore, see images below. Note that this does not happen when the Toolbox is closed.

Note that this does not happen in the current development version.

Desktop:

Resized window to mobile width:

gappc · 2024-09-12T17:01:55Z

Console warnings appear when dataset is opened, seems to be related to Toolbox changes, see image below.

Example URL: BASE_URL/dataset/table/tourism/v1/Accommodation

gappc

Lots of changes, lots of new stuff, some problems encountered ;) please fix

gappc · 2024-09-12T11:07:06Z

databrowser/src/pages/datasets/overview/OverviewCardItem.vue

-          />
-          <OverviewTagAccess :dataset="dataset" />
+  <CardContainer no-padding>
+    <CardLinkContainer :to="tableLocation as string">


Casting tableLocation to string seems wrong (casting in general is a code smell and should be avoided if possible), because the type of tableLocation is DatasetLocationRoute, whose string representation is [object Object]. I'm pretty sure that was not intended.

The only reason why this works is, because DatasetLocationRoute and the to property of router-link used inside the CardLinkContainer have some object properties in common.

Furthermore, the assignment creates lots of warnings in the console (see image below). Please fix this.

gappc · 2024-09-12T11:08:42Z

databrowser/package.json

@@ -34,7 +34,8 @@
    "vue-hotjar-next": "^1.4.0",


vue-hotjar-next was removed as of 4f0dd6e, please rebase this PR

gappc · 2024-09-12T11:18:32Z

databrowser/package.json

@@ -34,7 +34,8 @@
    "vue-hotjar-next": "^1.4.0",
    "vue-i18n": "^9.2.2",
    "vue-json-pretty": "^2.2.3",
-    "vue-router": "^4.2.2"
+    "vue-router": "^4.2.2",
+    "vue3-cookies": "^1.0.6"


Please do not use cookies to store client-side data. Use localStorage / sessionStorage instead, like in the EditHint component.

gappc · 2024-09-12T11:33:26Z

databrowser/src/components/card/CardLinkContainer.vue

+
+<script setup lang="ts">
+defineProps<{
+  to: string;


I'd use RouteLocationRaw here as that is the type router-link expects, see e.g. InternalLink component.

RouteLocationRaw is a union type that also accepts strings. This makes the component more flexible in terms of the route locations it can handle.

gappc · 2024-09-12T11:38:20Z

databrowser/src/components/input/InputSearch.vue

    :label-placeholder="t('components.inputSearch.labelPlaceholder')"
+    label-button="Search"


The button text is already translated, I think hard-coding it here was maybe by mistake? ;)

gappc · 2024-09-12T16:36:17Z

databrowser/src/domain/datasets/ui/common/MainAndSubCategories.vue

+    category.subElements.map((item) => {
+      const pathArray = R.split('.', item.objectPath);
+      const existsProperty = R.path(pathArray, _data) !== undefined;
+
+      return (item.elements.visible = existsProperty);
+    });


The usage of map here is not advised, as you change the contents of category.subElements inside the map function, thus provoking side effects that should never be part of a map function.

Please use either category.subElements.forEach or iterate with a good old for - loop.

gappc · 2024-09-12T16:39:20Z

databrowser/src/domain/datasets/ui/tableView/load/tableCols.ts

@@ -22,8 +22,13 @@ export const computeTableCols = (
    return [];
  }

-  return view.elements.map<Column>((element) => {
+  const elements = useTableViewColsStore().showDeprecated


Please provide the value of useTableViewColsStore().showDeprecated as property of the function computeTableCols, instead of rereading it from the store.

That way, this function remains pure, which is something really nice to reason about.

gappc · 2024-09-12T16:41:42Z

databrowser/src/domain/datasets/ui/tableView/tableViewColsStore.ts

+  actions: {
+    setShowDeprecated(value: boolean) {
+      this.showDeprecated = value;
+    },
+  },
 });


This action is most probably not needed, see also comment on DatasetHeaderConfigPopup component here: #596 (comment)

gappc · 2024-09-12T16:44:26Z

databrowser/src/domain/datasets/ui/toolBox/toolBoxStore.ts

+
+const preferredToolboxVisibility = !mdAndLarger.value
+  ? 'false'
+  : cookies.get('isToolboxVisible');


Use localStore instead of cookies for client-only data. See EditHint component as an example

gappc · 2024-09-12T16:45:26Z

databrowser/src/domain/datasets/ui/toolBox/toolBoxStore.ts

+export const toggleToolboxVisibility = (isVisible: boolean) => {
+  const toolBoxStore = useToolBoxStore();
+  toolBoxStore.visible = isVisible;
+  cookies.set('isToolboxVisible', String(isVisible));
+};
+
+export const toggleShowAll = (showAll: boolean) => {
+  const toolBoxStore = useToolBoxStore();
+  toolBoxStore.settings.showAll = showAll;
+};
+
+export const toggleShowDeprecated = (showDeprecated: boolean) => {
+  const toolBoxStore = useToolBoxStore();
+  toolBoxStore.settings.showDeprecated = showDeprecated;
+};
+
+export const toggleShowReferences = (showReferences: boolean) => {
+  const toolBoxStore = useToolBoxStore();
+  toolBoxStore.settings.showReferences = showReferences;
+};


Please put all of this stuff inside the store as actions. Here it is the right thing to do!

gappc · 2024-09-19T12:35:02Z

@MatteoBiasi @a-crea thanks for the fixes, should I do another code review?

Another important thing about branch alignment: I saw that the current development branch and your PR branch august-2024-ui-and-usability-improvements divert after commit disabling main deployment with commit SHA 5517df97392a5d689870470ce581e798d62f3af1. You can see that when you open your branch in GitHub, where the following is stated (see image below):

"This branch is 30 commits ahead of, 12 commits behind"

This could be problematic for merging and tooling. I would be very grateful (and strongly recommend) if you could apply your changes on TOP of the current development branch.

That way we avoid possible merge problems that go unnoticed. Otherwise there will be double (same) commits (see image below), maybe stuff that was changed and is overwritten by your changes and all kinds of other nasty things. You can take a look at the result if you try to do the merge locally.

If you have any problems with rebase / merge or want to discuss this topic, just let me know 👍

gappc · 2024-09-23T19:58:57Z

@MatteoBiasi @a-crea @xleddyl guys, while i appreciate that you are ironing out this PR, I have some trouble with the way you provide your contributions.

First, this PR becomes very big, touching a lot of stuff. While I myself sometimes go down the unhappy route of big PRs, it remains to say that they are usually problematic in terms of code and feature changes and especially for code review and testing. I thought we agreed to not create massive PRs if there are not very good reasons.

Second, I'd like to ask you for the THIRD TIME to bring this PR up do date with the current development branch, without polluting the commit history. Or is there any good reason not to do so, that I cannot see?

Third, I see that you introduce / change project dependencies (previously it was a cookie library, now you update tailwind). While updating libraries is fine to some degree, if its not strictly necessary I'd ask you to not do that in such a massive PR. Finding the reason for bugs or degradations becomes hard when everything is mixed together - I know what I'm talking about from different perspectives. If you think you have to change dependencies, lets discuss it briefly.

@sseppi: whats your take on this?

…erties.

sseppi · 2024-09-24T07:45:45Z

@MatteoBiasi @gappc thank you for the contribution of both of you in trying to close this PR. In my point of view there is space for optimization, as always, but now we have to find the best and quickest way to fix and close this issue.

Since is the release of the features included in this PR is quite urgent, what about organizing a pair programming session to review the code together and close the PR?

@gappc is there a way to publish this PR in a dedicated staging environment, to share it with the customer care team and let them test and prepare the documentation we need for the clients?

gappc · 2024-09-24T08:15:15Z

@sseppi @MatteoBiasi the branch in its current state cannot be build, at least in my environment I get 2 errors when I run npm run build (after doing npm i)

The ouput is as follow:

src/components/card/CardLinkContainer.vue:9:5 - error TS2322: Type 'RouteLocationRaw | undefined' is not assignable to type 'RouteLocationRaw'.

9     :to="to"
      ~~~

  node_modules/vue-router/dist/vue-router.d.ts:1153:5
    1153     to: RouteLocationRaw;
             ~~
    The expected type comes from property 'to' which is declared here on type 'AllowedComponentProps & ComponentCustomProps & VNodeProps & RouterLinkProps & Record<string, unknown>'

src/domain/datasets/ui/detailView/DetailView.vue:22:9 - error TS2322: Type '({ from: string; url: string; } | null)[]' is not assignable to type 'ReferenceInfoToolBoxFetchUrlInfo[]'.

22         :references-urls="referencesUrls"
           ~~~~~~~~~~~~~~~~



Found 2 errors in 2 files.

Errors  Files
     1  src/components/card/CardLinkContainer.vue:9
     1  src/domain/datasets/ui/detailView/DetailView.vue:22

the first problem is "obvious" in a sense, that undefined cannot be used as route
the second error looks a bit odd, because as far as I can see, the types emerging from the code seem to be correct, but the compiler tells otherwise. I'm not sure what's the problem there

As soon as the problems get fixed, I can deploy the PR onto a temporary location where you can check its current state.

MatteoBiasi · 2024-09-24T09:20:03Z

@gappc The issues with the build have been resolved. Regarding the cookie package, the problem hadn't actually resurfaced; the code had been corrected using localStorage, but the package was forgotten and not removed. I've now removed it. So, everything is fine on that front as well.

gappc · 2024-09-24T09:41:51Z

@MatteoBiasi thx for the fixes, @sseppi you can find the deployed PR here: https://0.databrowser.gappc.net/

gappc · 2024-09-24T09:47:00Z

@MatteoBiasi please note that there seems to be a bug in the TableView filters, where the selectbox options don't show up. Maybe a problem of z-index (see image below). Example URL: https://0.databrowser.gappc.net/dataset/table/tourism/v1/Accommodation

sseppi · 2024-09-24T13:12:33Z

@MatteoBiasi thx for the fixes, @sseppi you can find the deployed PR here: https://0.databrowser.gappc.net/

@gappc thank you for the link! I shared it with the customer care, to test and document the new features.

sseppi · 2024-09-24T16:04:43Z

I use this comment of the PR, to list all issues I found while I was testing. Please find the link in the list below:

@MatteoBiasi can you please have a look at it?

MatteoBiasi · 2024-09-25T07:27:47Z

Thank you @gappc and @sseppi for the two reports. @sseppi, if it's fine with you, shall we give ourselves another 1-2 days to complete the bug reporting from your side and then do a round of fixes?

sseppi · 2024-09-26T08:03:14Z

Thank you @gappc and @sseppi for the two reports. @sseppi, if it's fine with you, shall we give ourselves another 1-2 days to complete the bug reporting from your side and then do a round of fixes?

@MatteoBiasi from mi side I did some testing yesterday and I inserted everything I found. Being the feature changes that big is difficult to be sure to have tested everything, but we need to go online till the end of the month. For that reason I would suggest to proceed fix everything we found and try to go online.

MatteoBiasi · 2024-09-27T15:49:01Z

@sseppi the fixes for the reported issues have been pushed. For #609, we’ve temporarily hidden the input while waiting for a final decision on what to do. Also, see the note in #602.

sseppi · 2024-09-27T16:47:57Z

@gappc I think the PR is ready to be reviewed

gappc · 2024-09-29T17:36:16Z

@sseppi @MatteoBiasi I've reviewed the changes. Although there are some minor bugs / issues (see below), I believe the PR is ready to be merged..

I would greatly appreciate it if we could address the findings mentioned below, perhaps in a dedicated issue, to ensure they are not overlooked. Lowering the quality of the code will make it harder to maintain and extend the codebase in the future. I'm confident that these issues can be easily fixed, so please don't hesitate to ask me if you need any assistance.

Bug / Issue list

Referenced attributes not shown when toggle is disabled

Detail view: referenced attributes are not shown when "Load referenced attributes" is disabled (see feedback from previous code review here)

Text for toggle could be misunderstood

Detail view toolbox: the text for the toggle could be misunderstood: "Show attributes which are referenced from other datasets". I think it is the other way around, something like "Show attribute values that are referenced from this record / dataset / (some better naming?)"

Search button on mobile

On mobile, the search button is still visible in detail / edit / raw view, but clicking on it has no useful effect. I understand that at the moment the search is not available in these views, but the button should be hidden in that case.

Popover vs Tooltip component

Popover vs Tooltip component (see feedback from previous code review here). I think we should try to stick with Popover component for the reasons outlined in the feedback.

Duplicate attributes shown in Toolbox

Duplicate attributes shown in Toolbox under "Export datasets" in detail view (see image below)

Additional left padding in table view filters

Additional left padding in table view filters (see image below)

Warnings in the console

Warnings in the console when opening the detail view of a record (see image below), e.g. BASE_URL/dataset/detail/tourism/v1/Accommodation/5E4BEFD440200CE4732A7FA30897C402

Inconsistent "Show deprecated fields" toggles

In detail / edit view, the "Show deprecated fields" toggles are not in sync (see image below).

Invalid HTML

Self-closing div is not valid HTML (see DatasetHeaderOverlay.vue)

TypeScript issues

Type casts

Don't do explicit type casts if not strictly necessary.

If a given type is unknown and you expect it to be an array, then test for that e.g. if (Array.isArray(value)) (see problematic code in SubCategories.vue here). TypeScript will understand what you're trying to do.

Usage of `any`

I saw that new code was introduced that uses the any type, e.g. in the file DatasetHeader.vue. Please avoid using any as much as possible. If you're not sure about the type of a variable, use unknown instead of any. If you're sure about the type, then use that type. If you have further trouble with TypeScript, then you can ask me any time, I'm confident we'll get the code working without any 99,5% of the time.

gappc · 2024-09-29T17:39:30Z

@sseppi @mrabans @MatteoBiasi @a-crea @xleddyl @pkritzinger @Mazzolintis you can view the state of the current PR here: https://0.databrowser.gappc.net/

RudiThoeni · 2024-09-30T07:58:21Z

@sseppi should this PR be merged now or let us wait for this minor issues (@gappc ) listed to be fixed?
I also fear that they are overlooked if not fixed yet........

sseppi · 2024-09-30T15:43:58Z

@MatteoBiasi how long will it take to fix the issuee mentioned in the yesterday's comment: #596 (comment)

@gappc it seems that there is some SSL problem, the databrowser the link you shared doesn't load the datasets:

MatteoBiasi · 2024-09-30T17:50:58Z

@sseppi we estimate to fix them by Thursday

gappc · 2024-09-30T18:00:52Z

@sseppi you wrote:

it seems that there is some SSL problem, the databrowser the link you shared doesn't load the datasets:

The error response comes from the ODH testing environment, e.g. https://api.tourism.testingmachine.eu/v1/MetaData?pagesize=1000 returns with a TLS error.

The current databrowser test environment seems also not to work, see https://databrowser.opendatahub.testingmachine.eu/dataset-overview. @RudiThoeni any ideas what the problem could be?

@ALL as I mentioned, in my opinion the PR can be merged in its current state, there are no blockers or major bugs remaining. I would just appreciate if the list of remaining minor bugs / issues is not lost, so we don't forget about them.

Thanks all for your work and time 👍

RudiThoeni · 2024-10-01T09:29:52Z

@sseppi @gappc all works again it was a misconfiguration of the test proxy server thx to @clezag for fixing it

RudiThoeni · 2024-10-02T12:34:24Z

@MatteoBiasi I will merge this PR, for the issues mentioned by @gappc please open another PR

sseppi reviewed Sep 12, 2024

View reviewed changes

gappc self-requested a review September 12, 2024 08:10

gappc requested changes Sep 12, 2024

View reviewed changes

MatteoBiasi self-assigned this Sep 19, 2024

Matteo Biasi and others added 14 commits September 23, 2024 22:30

feat: completed issues 526, 575 a and started 525

aa619dc

feat: add reference color and tag style

7d10aad

feat: add tag type

f631ab9

feat: complete issues 533 and 532

55d89eb

fix: missing components after lint:fix to complete issues 533 and 532

33a122a

feat: implement select group options

b6b82b8

feat: issue 525 completed. Dataset selector.

0667057

feat: card layout improvement, search input and select

aff8bf0

feat: card layout improvement, search input and select

c21eed2

feat: add sub sections in categories. Add support for additional prop…

b1442d3

…erties.

feat: new navigation layout

d8efc4c

feat: new navigation layout

9a0a3da

fix: minor Table View optimizations

691abbb

feat: minor GUI optimizations to new home

f8bceb3

fix: fixed build issues and remove unnecessary packages

a703a01

fix: added conditional display to tableLocation viewer

32cb8a5

fix: optimized CardLinkContainer display without link

1a1f842

sseppi mentioned this pull request Sep 25, 2024

Implement first draft of additional properties for ODHActivityPoi #555

Closed

Matteo Biasi added 2 commits September 27, 2024 17:40

fix: fixed issues noi-techpark#602, noi-techpark#604, noi-techpark#609

c03c963

fix: added Issue 602 note

b00ac0e

gappc mentioned this pull request Sep 29, 2024

Search box in the Detail Views doesn't work #609

Closed

gappc approved these changes Sep 30, 2024

View reviewed changes

RudiThoeni merged commit 048cccf into noi-techpark:development Oct 2, 2024
3 checks passed

a-crea mentioned this pull request Oct 3, 2024

Fix/optimizations and fixes pr 596 #612

Merged

gappc mentioned this pull request Oct 3, 2024

Warnings in the console in Detail view #614

Closed

		:label-placeholder="t('components.inputSearch.labelPlaceholder')"
		label-button="Search"

August 2024 UI and usability improvements #596

August 2024 UI and usability improvements #596

Conversation

MatteoBiasi commented Sep 4, 2024

Choose a reason for hiding this comment

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024 • edited Loading

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024

gappc commented Sep 12, 2024 • edited Loading

gappc commented Sep 12, 2024

gappc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gappc commented Sep 19, 2024

gappc commented Sep 23, 2024

sseppi commented Sep 24, 2024

gappc commented Sep 24, 2024

MatteoBiasi commented Sep 24, 2024

gappc commented Sep 24, 2024

gappc commented Sep 24, 2024

sseppi commented Sep 24, 2024 • edited Loading

sseppi commented Sep 24, 2024 • edited Loading

MatteoBiasi commented Sep 25, 2024

sseppi commented Sep 26, 2024

MatteoBiasi commented Sep 27, 2024

sseppi commented Sep 27, 2024

gappc commented Sep 29, 2024

Bug / Issue list

Referenced attributes not shown when toggle is disabled

Text for toggle could be misunderstood

Search button on mobile

Popover vs Tooltip component

Duplicate attributes shown in Toolbox

Additional left padding in table view filters

Warnings in the console

Inconsistent "Show deprecated fields" toggles

Invalid HTML

TypeScript issues

Type casts

Usage of any

gappc commented Sep 29, 2024

RudiThoeni commented Sep 30, 2024

sseppi commented Sep 30, 2024

MatteoBiasi commented Sep 30, 2024

gappc commented Sep 30, 2024

RudiThoeni commented Oct 1, 2024

RudiThoeni commented Oct 2, 2024

gappc commented Sep 12, 2024 •

edited

Loading

gappc commented Sep 12, 2024 •

edited

Loading

sseppi commented Sep 24, 2024 •

edited

Loading

sseppi commented Sep 24, 2024 •

edited

Loading

Usage of `any`