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.

Sign up for GitHub

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

Jump to bottom

[MB-7680] TOO can identify newest orders uploaded in TOO UI #6811

Merged
merged 10 commits into from
Jun 17, 2021
Merged

[MB-7680] TOO can identify newest orders uploaded in TOO UI #6811

merged 10 commits into from
Jun 17, 2021

Conversation

jenniferlynparsons
Copy link
Contributor

@jenniferlynparsons jenniferlynparsons commented Jun 15, 2021 •
edited
Loading

Description

TOO sees the creation date for the uploaded orders on the document viewer, sorted by creation date.

Reviewer Notes

The Go tests are broken and I'm not sure why. I've added a second document creation call to the seed data generator functions so that's where the culprit might be.

Setup

Add any steps or code to run in this section to help others prepare to run your code:

make server_run
make office_client_run

// Some changes are visible with
make storybook

Code Review Verification Steps

  • If the change is risky, it has been tested in experimental before merging.
  • Code follows the guidelines for Logging
  • The requirements listed in Querying the Database Safely have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #g-database
    • Secure migrations have been tested following the instructions in our docs
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Jira acceptance criteria been met for this change?

References

Screenshots

Screen Shot 2021-06-16 at 2 35 43 PM
Screen Shot 2021-06-16 at 2 35 57 PM

@robot-mymove
Copy link

robot-mymove commented Jun 15, 2021 •
edited
Loading

Messages
📖 🔗 MB-7680

Generated by 🚫 dangerJS against bf22a16

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.41 MB 46.41 MB 0 B (0.00%)

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.41 MB 46.41 MB 0 B (0.00%)

@jenniferlynparsons jenniferlynparsons marked this pull request as ready for review June 16, 2021 18:36
@jenniferlynparsons jenniferlynparsons requested review from a team June 16, 2021 18:36
@jenniferlynparsons jenniferlynparsons self-assigned this Jun 16, 2021
@duncan-truss
Copy link
Contributor

duncan-truss commented Jun 16, 2021 •
edited
Loading

@jenniferlynparsons If we have a very long file name, we are never going to see the Added on date displayed. Maybe we can change the html so that we always display the Added on date and only truncate the file name portion not the entire paragraph contents.

Screen Shot 2021-06-16 at 5 25 11 PM

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.4 MB 46.4 MB 0 B (0.00%)

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.4 MB 46.4 MB 0 B (0.00%)

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.4 MB 46.4 MB 0 B (0.00%)

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.4 MB 46.4 MB 0 B (0.00%)

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.4 MB 46.4 MB 0 B (0.00%)

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.4 MB 46.4 MB 0 B (0.00%)

duncan-truss
duncan-truss reviewed Jun 17, 2021
View reviewed changes
src/components/DocumentViewer/DocumentViewer.test.jsx Outdated
});

it('renders the title bar with the correct props', () => {
expect(component.find('[data-testid="documentTitle"]').text()).toBe('Test File 4.gif   - Added on 16 Jun 2021');
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with the extra whitespace in the title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spacing weirdness that I've now replaced with some CSS padding.

duncan-truss
duncan-truss reviewed Jun 17, 2021
View reviewed changes
src/components/DocumentViewer/DocumentViewer.jsx Outdated
@@ -72,14 +75,17 @@ const DocumentViewer = ({ files }) => {

const selectedFilename = filenameFromPath(selectedFile.filename);

const selectedFileDate = moment(selectedFile.createdAt).format('DD MMM YYYY');
Copy link
Contributor

Choose a reason for hiding this comment

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

We likely have a formatter defined for this in src/shared/formatters.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the main reason to use the formatDate method in formatters is to provide default values? That makes sense.

duncan-truss
duncan-truss reviewed Jun 17, 2021
View reviewed changes
src/components/DocumentViewer/Menu/Menu.jsx Outdated
@@ -25,12 +26,16 @@ const DocViewerMenu = ({ isOpen, files, handleClose, selectedFileIndex, handleSe
const itemClasses = classnames(styles.menuItemBtn, {
[styles.active]: i === selectedFileIndex,
});
const filename = filenameFromPath(file.filename);
const fileName = filenameFromPath(file.filename);
const fileDate = moment(file.createdAt).format('DD-MMM-YYYY');
Copy link
Contributor

Choose a reason for hiding this comment

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

similar point here about an existing formatter

duncan-truss
duncan-truss reviewed Jun 17, 2021
View reviewed changes
src/components/DocumentViewer/DocumentViewer.test.jsx
@@ -125,6 +57,14 @@ describe('DocumentViewer component', () => {
expect(menu.prop('files')).toBe(mockFiles);
});

it('renders the file creation date with the correctly sorted props', () => {
expect(component.find('li button').at(0).text()).toBe('Test File 4.gif Uploaded on 16-Jun-2021');
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

duncan-truss
duncan-truss approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@duncan-truss duncan-truss left a comment

Choose a reason for hiding this comment

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

Looks good!

@github-actions
Copy link

Bundle difference

Old size New size Diff
46.41 MB 46.41 MB 0 B (0.00%)

samadden
samadden approved these changes Jun 17, 2021
View reviewed changes
Copy link

@samadden samadden left a comment

Choose a reason for hiding this comment

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

LGTM

@jenniferlynparsons jenniferlynparsons merged commit 9d585a5 into master Jun 17, 2021
@jenniferlynparsons jenniferlynparsons deleted the jp-MB-7680-TOO-can-identify-newest-orders-uploaded-in-TOO-UI branch June 17, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duncan-truss duncan-truss duncan-truss approved these changes

@samadden samadden samadden approved these changes

@monfresh monfresh Awaiting requested review from monfresh

@donaldthai donaldthai Awaiting requested review from donaldthai

Assignees

@jenniferlynparsons jenniferlynparsons

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jenniferlynparsons @robot-mymove @duncan-truss @monfresh @donaldthai @samadden