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

Apply latest changes from datalad-catalog #50

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Conversation

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 1, 2023

I see the border around the dataset title does not display nicely for titles that span multiple lines. will update this.

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 4, 2023

I see the border around the dataset title does not display nicely for titles that span multiple lines. will update this.

Updated

Copy link

@adswa adswa left a comment

Choose a reason for hiding this comment

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

I have explored your rendered demo using Firefox 102.7.0esr (64-bit), and I went through every linked issue. Overall, it looks like the linked issues are indeed fixed by all these changes, I just had two observations noted below:

For issue #33, I confirmed that I can link the funding tab (e.g., https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/funding, which leads to "funding"), but this doesn't seem to work for the "SFB1451" tab:
https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/SFB1451 (this links to subdatasets).

For issue #43 I did not find non-https urls, likely because I don't know where to look or because non https urls aren't hyperlinks anymore due to #39?

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 5, 2023

Thanks @adswa!

For issue #33, I confirmed that I can link the funding tab (e.g., https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/funding, which leads to "funding"), but this doesn't seem to work for the "SFB1451" tab:
https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/SFB1451 (this links to subdatasets).

Indeed, this is the same for me. Thanks for spotting this, I will look into it.

For issue #43 I did not find non-https urls, likely because I don't know where to look or because non https urls aren't hyperlinks anymore due to #39?

Yup I already "eliminated" the non-http urls via datalad/datalad-catalog@498c278. You can see the previous state in the current production site: https://psychoinformatics-de.github.io/sfb1451-projects-catalog/

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 5, 2023

For issue #33, I confirmed that I can link the funding tab (e.g., https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/funding, which leads to "funding"), but this doesn't seem to work for the "SFB1451" tab:
https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/SFB1451 (this links to subdatasets).

Looks like a simple capitalisation issue. The tab_param here https://github.com/datalad/datalad-catalog/blob/main/datalad_catalog/catalog/assets/app_component_dataset.js#L514 needs to be changed to tab_param.toLowerCase()

@mslw
Copy link
Collaborator

mslw commented Oct 5, 2023

Thanks for the review @adswa , and naturally thanks for the rapid-fire changes @jsheunis !

I also played with the demo - I think especially the "back" button and improved filtering make it feel decidedly snappier.

I understand that this is ready to merge? If so, I will do it later today, but I wanted to push one more metadata update first.

There is one minor thing:

For issue #43 I did not find non-https urls, likely because I don't know where to look or because non https urls aren't hyperlinks anymore due to #39?

Yup I already "eliminated" the non-http urls via datalad/datalad-catalog@498c278. You can see the previous state in the current production site: https://psychoinformatics-de.github.io/sfb1451-projects-catalog/

which I feel was a bit "throwing the baby out with the bathwater" - I've already suggested in #43 that instead of requiring urlparse(url).scheme in ("http", "https") (sorry for using Python examples when talking about JS...) to showing download button, my instinct would be to just say urlparse(url).scheme != "", but I don't want to override the change with another hasty change. In any case, that's not a blocker for me, and from my memory I'm not sure if we have any useful non-html links in the SFB metadata right now...

edit: corrected from "netloc" to "scheme", and "html" to "http" to avoid future confusion

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 5, 2023

I understand that this is ready to merge?

I want to push a couple of changes still, including a fix for the first issue that @adswa highlighted. @mslw I will tag you here when done.

Wrt your second point, I agree the current state is a somewhat blunt "fix". I do think the UX is important though, and having a download button that always fails is bad UX. I would suggest more fit-for-purpose functionality, such as having a scheme for interpretation and having different renderings and UX accordingly. I think this can probably be explored further in a separate datalad-catalog issue, though, and then an eventual fix can be merged in here separately if/when it is addressed. WDYT?

@mslw
Copy link
Collaborator

mslw commented Oct 5, 2023

Wrt your second point, I agree the current state is a somewhat blunt "fix". I do think the UX is important though, and having a download button that always fails is bad UX. I would suggest more fit-for-purpose functionality, such as having a scheme for interpretation and having different renderings and UX accordingly. I think this can probably be explored further in a separate datalad-catalog issue, though, and then an eventual fix can be merged in here separately if/when it is addressed. WDYT?

Yes, absolutely.

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 6, 2023

I missed this issue previously: #22. Will work this into the PR as well.

@tmheunis
Copy link

tmheunis commented Oct 6, 2023

I have reviewed the issues in this PR, and have also done some other spot checks. For issues applicable to both the sfb-catalog and datalad-catalog I have created specific issues in the datalad-catalog repository and listed them below in number 3.

1. Download link available for inaccessible files

In the dataset, https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest, click on content tab and then go to any of the files under Z03. For e.g. if you click on P000840, https://data.inm-icf.de/Z03/P000840_dicom.tar, this file cannot be accessed. Should this then display as a downloadable file? This issue in datalad-catalog seems to be related datalad/datalad-catalog#380, but the question still remains what needs to be done for the SFB catalog specifically?

2. URL based tab navigation still incomplete

E.g. If I click on 'funding' and copy the url it does not include the tab parameter in the url. The same applies for the 'content' tab. If I do include the 'funding' tab parameter in the url it correctly opens that tab, but if I then click on 'content', for instance, the url still displays 'funding'. The url should end with 'content' if I am on the 'content' tab, and end on 'funding' if I am on the 'funding' tab. Link issue #33.

3. Other issues created on datalad-catalog, applicable here too

datalad/datalad-catalog#383
datalad/datalad-catalog#384
datalad/datalad-catalog#385
datalad/datalad-catalog#386
datalad/datalad-catalog#387
datalad/datalad-catalog#388
datalad/datalad-catalog#389
datalad/datalad-catalog#390

@mslw
Copy link
Collaborator

mslw commented Oct 6, 2023

Thanks @tmheunis ! I can speak to one question directly:

1. Download link available for inaccessible files

In the dataset, https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest, click on content tab and then go to any of the files under Z03. For e.g. if you click on P000840, https://data.inm-icf.de/Z03/P000840_dicom.tar, this file cannot be accessed. Should this then display as a downloadable file? This issue in datalad-catalog seems to be related datalad/datalad-catalog#380, but the question still remains what needs to be done for the SFB catalog specifically?

The source for these URLs is a tabby file with dataset description prepared by the Z03 project members. In particular, the urls are accessible only within FZJ internal network and require additional authentication. Personally, I think the links should be displayed, if only as an information.

@jsheunis
Copy link
Collaborator Author

jsheunis commented Oct 6, 2023

Thanks for the review and all the catalog issues, @tmheunis!

Some comments after my latest commits:

For issue #33, I confirmed that I can link the funding tab (e.g., https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/funding, which leads to "funding"), but this doesn't seem to work for the "SFB1451" tab:
https://jsheunis.github.io/sfb1451-projects-catalog/#/dataset/9964001a-5e8c-5c70-aedb-55141cca07c0/latest/SFB1451 (this links to subdatasets).

The capitalization issue that @adswa pointed out is now fixed and included in this PR.

@mslw @tmheunis, regarding:

The source for these URLs is a tabby file with dataset description prepared by the Z03 project members. In particular, the urls are accessible only within FZJ internal network and require additional authentication. Personally, I think the links should be displayed, if only as an information.

I'm also fine with this approach for the current goal, and then to use datalad/datalad-catalog#380 to figure the challenge out for the longer term.

@tmheunis, re:

2. URL based tab navigation still incomplete

E.g. If I click on 'funding' and copy the url it does not include the tab parameter in the url. The same applies for the 'content' tab. If I do include the 'funding' tab parameter in the url it correctly opens that tab, but if I then click on 'content', for instance, the url still displays 'funding'. The url should end with 'content' if I am on the 'content' tab, and end on 'funding' if I am on the 'funding' tab. Link issue #33.

Good point, I agree. I have updated this PR description at the top to indicate that the PR does not fully close #33, and I have created this issue in datalad-catalog to address the remaining problem: datalad/datalad-catalog#391

@mslw regarding:

I missed this issue previously: #22. Will work this into the PR as well.

as you saw in the linked issue there is a screenshot of a proposed solution. Once we get consensus on the styling, that can also become a separate PR, i.e. it does not necessarily have to be incorporated into this one.

I believe all other issues that became relevant in the course of this PR review process have now received their own tracking issues, either in this repo or in datalad-catalog. So I think if you are happy with the current state @mslw, you are welcome to merge. Fixes for the issues that remain open here, and any further changes in datalad-catalog, can then become part of a new PR.

@mslw
Copy link
Collaborator

mslw commented Oct 9, 2023

Thank you again @jsheunis this is a lot of improvements!

Given the green light in your latest comments, and reassured by the fact that more people looked into it, I'm merging. Thx all!

@mslw mslw merged commit 5405ea9 into sfb1451:main Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants