-
Notifications
You must be signed in to change notification settings - Fork 1
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
Per 9139 publish modal shows extra slash #305
Conversation
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.
This seems to work well! I left some notes about changes I'd like to be made to the test.
).toBeTruthy(); | ||
}); | ||
|
||
it('adds only one slash in the url before p/archive', () => { |
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.
I think this test might be a bit too specific; what we probably want is to make sure that there are no duplicate slashes in general. Instead of testing in one place (before the p/archive
), I think it might make more sense to test the entire URL for double slashes (or rather the pathname of the URL since the https://
bit at the beginning contains double slashes).
To make this testing easy we can take the string returned from pipe.transform()
and put it into a URL object and test the pathname:
const url = new URL(pipe.transform(folder));
expect(url.pathname).not.toContain("//");
a16395c
to
1e19dd2
Compare
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.
Looks good.
96f438c
to
ec2fa8b
Compare
Stepts to test:
1.Select a record.
2. Click share
3. The generated url should have only one slash before p/archive