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

Rename lists to views #1554

Merged
merged 3 commits into from
Sep 30, 2023
Merged

Rename lists to views #1554

merged 3 commits into from
Sep 30, 2023

Conversation

kaulfield23
Copy link
Contributor

Description

This PR fixes a bug where it breaks localization in view

Screenshots

image

Changes

  • Changes lists to views

Notes to reviewer

Related issues

Undocumented

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

The changes to the makeMessages() prefix looks good to me, but I don't understand why we would need to change the test. They should be unrelated, and to me the test looked right before (using "list"). If that test failed, I believe that it's either flaky (retry it) or the code it's testing is wrong (fix it).

@@ -78,7 +78,7 @@ test.describe('View detail page', () => {
expect(columnPostLogs).toHaveLength(2);

// Expect that correctly localised strings sent when posting
expect(viewPostLogs[0].data?.title).toEqual('New list');
expect(viewPostLogs[0].data?.title).toEqual('New View');
Copy link
Member

Choose a reason for hiding this comment

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

This should not have to change. The correct text to use for a newly created list is "New list". Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test failed there and result said it was expecing to have New list but the result was New View so I fixed it to New View @richardolsson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I fix it as before and retry the test?

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

I'm not sure why I didn't already review this, because it looks fine and simple!

@richardolsson richardolsson merged commit b144676 into main Sep 30, 2023
4 checks passed
@richardolsson richardolsson deleted the undocumented/wrong-path-name branch September 30, 2023 04:23
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.

2 participants