-
Notifications
You must be signed in to change notification settings - Fork 0
feat: support demo theme menu, update Cypress theme tests. #140
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Caution
Changes requested ❌
Reviewed everything up to 48a3885 in 1 minute and 56 seconds. Click for details.
- Reviewed
395lines of code in11files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. cypress/integration/theme/davids/davidsMain.cy.js:4
- Draft comment:
Test is clear; consider extracting the repeated selectors (e.g. for the nav menu icon and demo themes) into constants or helper functions to improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. cypress/integration/theme/davids/davidsSkillFilter.cy.js:12
- Draft comment:
This test is currently skipped; ensure to re-enable it once the referenced issue (#40) is resolved to avoid regression. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Tests are typically skipped for good reasons, with tracking issues. The author knows the test is skipped since they added the .skip() and the issue reference. Suggesting to re-enable it doesn't add value - they'll do that when the issue is resolved. This falls under "do not ask authors to verify/ensure things" and "do not make purely informative comments". Maybe keeping track of skipped tests is important for test coverage and technical debt tracking? The issue link already serves as tracking. Adding a comment doesn't provide additional value and could become stale. Delete the comment. The skip is intentional with proper tracking via the linked issue.
3. cypress/integration/theme/default/defaultMain.cy.js:4
- Draft comment:
Test steps are clear; as more similar theme tests are added, consider abstracting common navigation steps into a shared helper to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. cypress/pages/navigationMenuPage.cy.js:1
- Draft comment:
The navigation page object is well-structured; consider expanding it with additional methods if more navigation items are introduced in the future. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. cypress/pages/themeDavidsPage.cy.js:1
- Draft comment:
The ThemeDavidsPage page object is clean and easy to follow. No issues noted. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the code is clean and easy to follow, which is not useful for the PR author.
6. src/app/components/NavPrimary.tsx:235
- Draft comment:
The new demo themes submenu is implemented correctly. Consider resetting the 'demoThemesOpen' state (in addition to closing the main drawer) when a submenu item is clicked to prevent any stale UI state. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment points out a real UX consideration - when reopening the drawer, users might expect the submenu to start in a collapsed state. However, this is a minor UX preference rather than a clear bug or issue. The current behavior (preserving submenu state) could be equally valid and might even be preferred by some users. There's no strong technical reason to prefer one behavior over the other. The current behavior might actually be intentional - preserving UI state between opens can improve UX by maintaining context. Also, the unmountOnExit prop means there's no performance impact. While preserving state could be intentional, the suggestion is still a valid UX consideration that the author may want to evaluate. The change would be simple to implement if desired. The comment should be deleted because it's suggesting a UX preference rather than fixing a clear issue. Both behaviors (resetting vs preserving submenu state) are valid approaches.
7. src/app/demo/[themeName]/page.test.tsx:22
- Draft comment:
Metadata tests have been updated to use the theme's display name from 'themeDefinitions', which improves consistency in titles. The test appears accurate. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating that metadata tests have been updated and that the test appears accurate. It doesn't provide any actionable feedback or suggestions for improvement.
8. src/app/demo/[themeName]/pdf/page.test.tsx:30
- Draft comment:
PDF metadata tests have been updated to reflect the display names from themeDefinitions. The expectations correctly match the updated metadata format. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, describing what has been done in the PR without providing any actionable feedback or suggestions. It doesn't ask for any specific changes or confirm any potential issues.
9. src/app/demo/[themeName]/pdf/page.tsx:15
- Draft comment:
The PDF page metadata generation now correctly uses themeDefinitions to obtain the display name for the title, ensuring consistency with the rest of the application. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what the code change does without suggesting any improvements or asking for clarifications. It doesn't align with the rules for useful comments.
10. src/lib/secureHtmlParser.ts:64
- Draft comment:
The addition of an 'isBrowser' check enhances SSR safety for DOM manipulations. The innerHTML handling appears cautious; consider verifying that the domNode reliably supports the 'innerHTML' property in all intended cases. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_eIpGPYZYNwU3NEjD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for submitting a PR! Please review & check the boxes below:
Description
I forgot to bring over some Cypress tests from the theme package repo. These are brought over now, and I added a navigation menu & submenu support to showcase the demo themes.
Steps to Test
Spin up on local, visit the nav menu and you should see demo themes accessible there:
Important
Adds demo theme menu in navigation, updates Cypress tests, improves theme metadata generation, and enhances HTML parsing security.
NavPrimary.tsxusingCollapsefor expandable list.SubmenuHeaderandSubmenuItemcomponents for theme navigation.davidsMain.cy.jsanddefaultMain.cy.js.davidsSkillFilter.cy.jsfor skill filter functionality.NavigationMenuPageandThemeDavidsPageclasses for page object model.generateMetadatainpage.tsxandpdf/page.tsxto use theme-specific names and descriptions.page.test.tsxandpdf/page.test.tsxto verify metadata generation.secureHtmlParser.tsto check for browser environment before parsing.hrefandsrcattributes to blockjavascript:anddata:URLs.This description was created by
for 48a3885. You can customize this summary. It will automatically update as commits are pushed.