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

Harvest UI tweaks #2033

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

hudson-newey
Copy link
Member

@hudson-newey hudson-newey commented Jan 25, 2023

Harvest UI tweaks

Following a user demo, users expressed that the Harvest UI needed some adjustments

Changes

  • Fixes bugs in folder grid alignment
  • Drop down chevrons are now not shown if there is no expandable content
  • Mappings are now inherited from their parent folders
  • Root mapping button is now not shown if there are no Harvest items directly underneath it
  • Changes terminology for a few UI elements
    • "Change mappings and UTC offset" is now "Add mappings and UTC offset"
    • Adds sidebar tip "You don't have to change anything for files that have no problems"
  • Adds action buttons / hyperlinks to action on tips in sidebar
  • Adds an extra tip to differentiate between duplicate files in the current upload and previous uploads
  • Shows the site ID in a tooltip for mappings when the user hovers over
  • UTC offset now shows relevant offsets to the site location at the top of the dropdown box
  • Creates tests for folder row component
  • Creates tests for file row component
  • Creates tests for UTC offset component
  • Adds tests to metadata review component to ensure mappings are inherited correctly
  • Updates dependency @angular-devkit/build-angular
  • Removes deprecated usage of ~ in scss imports
  • Modified association decorator to not always use cached model. It now checks that the cached model has the same id as the requested model before returning the cached model. If the two id's do not match, the model is fetched from the API, and the cached model is updated (Currently failing tests)

Problems

This is a draft pull request, I am still debugging, refining, and searching for new bugs.

Issues

Fixes: #1970

Visual Changes

Site id tool tip on hover:
image

Suggested UTC offsets in dropdown box:
image

Chevron does not show for single line errors:
image

New sidebar terminology & action hyperlinks:
image

Final Checklist

  • Assign reviewers if you have permission
  • Assign labels if you have permission
  • Link issues related to PR
  • Ensure project linter is not producing any warnings (npm run lint)
  • Ensure build is passing on all browsers (npm run test:all)
  • Ensure CI build is passing
  • Ensure docker container is passing (docs)

@hudson-newey hudson-newey added bug Something isn't working enhancement New feature or request work in progress Pull request that is currently a WIP labels Jan 25, 2023
@hudson-newey hudson-newey self-assigned this Jan 25, 2023
@hudson-newey hudson-newey added dependencies Pull requests that update a dependency file design A change to the design/look of the website or a component labels Jan 25, 2023
Copy link
Member Author

@hudson-newey hudson-newey left a comment

Choose a reason for hiding this comment

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

This PR is currently a WIP

What I have to complete before review

public constructor(private injector: Injector) {}

public ngOnChanges(): void {
// to inherit mappings values among ancestor items, if the parent item gains a Harvest Mapping, then all the child items should too
Copy link
Member Author

Choose a reason for hiding this comment

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

This method is currently faulty
Since folder rows only have knowledge of their parents, it is not possible to make rows inherit their parents mappings before they are shown on the DOM.

Therefore, I either need to change this.rows to be a doubly linked list (with knowledge of the children, so that I can recurse down the tree when a mapping is changed on a parent item), or move this logic to where the mappings change request is sent to the api (where it does have knowledge of the full model).

@@ -355,4 +355,7 @@ describe("MetadataReviewComponent", () => {
discardPeriodicTasks();
}));

it("should inherit mappings from parent item if harvest item mappings are not set", fakeAsync(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This unit test needs to be completed

@@ -84,13 +93,19 @@ interface ValidationMessage {
></fa-icon>
</div>
</div>
<ng-container *if="updateChevronState(validationsContainer)"></ng-container>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a hacky workaround I had to implement because change detection wasn't firing for *ngFor changes

Copy link
Member Author

Choose a reason for hiding this comment

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

This has now been resolved. It seems to have been an issue with the logic in my updateChevronState method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file design A change to the design/look of the website or a component enhancement New feature or request work in progress Pull request that is currently a WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harvest: metadata review screen UI tweaks
1 participant