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

August 2024 UI and usability improvements #596

Conversation

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

// FIXME: ADD FOR TESTING 533
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@gappc gappc self-requested a review September 12, 2024 08:10
@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

@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
Copy link
Collaborator

gappc commented Sep 12, 2024

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:
image

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

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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.

image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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

Referenced attributes visible (correct)
image

Referenced attributes not visible at all (bug?)
image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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

image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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

image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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

image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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

image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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:
image

Resized window to mobile width:
image

@gappc
Copy link
Collaborator

gappc commented Sep 12, 2024

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

image

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.

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

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

Choose a reason for hiding this comment

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

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.

image

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

Choose a reason for hiding this comment

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

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

@@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


<script setup lang="ts">
defineProps<{
to: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Comment on lines 61 to 66
category.subElements.map((item) => {
const pathArray = R.split('.', item.objectPath);
const existsProperty = R.path(pathArray, _data) !== undefined;

return (item.elements.visible = existsProperty);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

return view.elements.map<Column>((element) => {
const elements = useTableViewColsStore().showDeprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines 21 to 20
actions: {
setShowDeprecated(value: boolean) {
this.showDeprecated = value;
},
},
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Comment on lines 30 to 49
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;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@gappc
Copy link
Collaborator

gappc commented Sep 19, 2024

@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"

image

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.

image

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

@MatteoBiasi MatteoBiasi self-assigned this Sep 19, 2024
@gappc
Copy link
Collaborator

gappc commented Sep 23, 2024

@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?

@sseppi
Copy link
Contributor

sseppi commented Sep 24, 2024

@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
Copy link
Collaborator

gappc commented Sep 24, 2024

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

gappc commented Sep 24, 2024

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

@gappc
Copy link
Collaborator

gappc commented Sep 24, 2024

@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

image

@sseppi
Copy link
Contributor

sseppi commented Sep 24, 2024

@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
Copy link
Contributor

sseppi commented Sep 24, 2024

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
Copy link
Collaborator Author

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
Copy link
Contributor

sseppi commented Sep 26, 2024

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
Copy link
Collaborator Author

@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
Copy link
Contributor

sseppi commented Sep 27, 2024

@gappc I think the PR is ready to be reviewed

@gappc
Copy link
Collaborator

gappc commented Sep 29, 2024

@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)

image

Additional left padding in table view filters

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

image

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

image

Inconsistent "Show deprecated fields" toggles

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

image

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.

Other problematic instances:

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
Copy link
Collaborator

gappc commented Sep 29, 2024

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

@RudiThoeni
Copy link
Member

@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
Copy link
Contributor

sseppi commented Sep 30, 2024

@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:
Screenshot from 2024-09-30 17-41-26

@MatteoBiasi
Copy link
Collaborator Author

@sseppi we estimate to fix them by Thursday

@gappc
Copy link
Collaborator

gappc commented Sep 30, 2024

@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
Copy link
Member

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

@RudiThoeni
Copy link
Member

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

@RudiThoeni RudiThoeni merged commit 048cccf into noi-techpark:development Oct 2, 2024
3 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.

6 participants