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

Release v1.4.0 #25

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Release v1.4.0 #25

merged 1 commit into from
Oct 15, 2024

Conversation

GormFrank
Copy link
Collaborator

No description provided.

Copy link
Contributor

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

I reviewed the diff between commit 9fa2388 and v1.3.0 (which is a total of 2 commits) and I did checked the release note.

So far so good. Note that I remark that there is no demo/test that are illustrating all functionality. For example, there is additional feature available by providing custom HTML template that are not demoed. Not a blocker, but the working example should be improved (Enlarge its code coverage) or add unit testing for the next release of the search ui.

Request change is about the item "Patch: API - This is to fix API calls for Analytics, which would fail when cookies are turned off". I am unable to link that change with any of the code change. How can I validate that item by looking at the search-ui source code? Thanks for providing me the line of code illustrating that change.

@GormFrank
Copy link
Collaborator Author

@duboisp It is this line, illustrated in the recently merged PR: https://github.com/ServiceCanada/search-ui/pull/24/files#diff-5bf845b8b5d0fb5fc819cfcd30b5d4fc1701eb6726a6323e250ebdd9379c7a2fR435

It was observed that an error would be thrown when cookies are disable and that the code was trying to run requestContent.analytics.originLevel3 since in that case the .analytics is undefined. Adding the if statement prevents from having this error, and will not log analytics in the case of disabled cookies, with respect to the user's privacy settings.

Therefore, maybe the wording isn't quite optimal in the release notes: "This is to fix API calls for Analytics, which would fail when cookies are turned off", it could be instead "This is to fix search functionalities when cookies are turned off"

@GormFrank GormFrank requested a review from duboisp October 10, 2024 12:07
@duboisp
Copy link
Contributor

duboisp commented Oct 10, 2024

Thanks for the explanation. I am going to do a quick functional test now

Copy link
Contributor

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Please rebase your PR, the commit 9fa2388 is not there

@GormFrank
Copy link
Collaborator Author

GormFrank commented Oct 10, 2024

Please rebase your PR, the commit 9fa2388 is not there

@duboisp Done!

@GormFrank GormFrank requested a review from duboisp October 10, 2024 17:58
@duboisp duboisp merged commit 709eb3b into ServiceCanada:main Oct 15, 2024
1 check passed
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