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

Improve return type of getPageTitle test helper #288

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Improve return type of getPageTitle test helper #288

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions addon/src/test-support/get-page-title.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
// Testem appends progress to the title...
// and there's no way to stop this at the moment

export function getPageTitle(doc: Document) {
export function getPageTitle(doc?: Document): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc should not be required because of doc || window.document.

Copy link
Contributor Author

@bertdeblock bertdeblock Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Current return type is string | false | undefined because of the current implementation. Using string and updating the implementation seems better. Technically, a breaking change though.

// In Fastboot context we get 2 title elements if we don't remove one from app/index.html
// In real world applications, it is mandatory to remove <title> from app/index.html
// We are keeping both for sake for testing browser and fastboot scenarios
const element = [
...(doc || window.document).querySelectorAll('head title'),
].pop();

return (
element &&
element instanceof HTMLTitleElement &&
element.innerText.trim().replace(/^\(\d+\/\d+\)/, '')
);
if (element instanceof HTMLTitleElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much easier to read, thank you!

return element.innerText.trim().replace(/^\(\d+\/\d+\)/, '');
}

return '';
}
Loading