-
Notifications
You must be signed in to change notification settings - Fork 530
Dataset page: Use params for clarity and efficiency #11611
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
Dataset page: Use params for clarity and efficiency #11611
Conversation
stevenwinship
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current performance tests will most likely not see a significant change due to the limited number of datasets being tested. I would like to re-run them once this pr is merged into develop. If the tests show improvement (perf or other tests) then we should make the changes to other areas. I don't want to hold up this pr to extend it. A new issue can be created once we run more performance tests.
|
Should be fixed - was missing a commit fixing errors in the editFileFragment.xhtml file. |
|
Looks good, nothing else stood out during testing. I'll merge once continuous integration finishes running. |

What this PR does / why we need it: This PR defines and uses jsf params in dataset.xhtml page and several things it includes. The intent was to improve efficiency and clarity. There should be no functional change.
Which issue(s) this PR closes:
Special notes for your reviewer: In some quick testing at QDR, I thought I could see some improvement in page load times with the changes here. If that can be confirmed by @landreev or @stevenwinship with more formal performance testing, this could be merged as is or expanded to include the dataverse and/or other pages.
Suggestions on how to test this: Regression and performance testing.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: