docs: include CDN information and links to examples #101
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @MinCrohn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request authored by MinCrohn. This PR primarily focuses on enhancing and refining the documentation, specifically the README.md and README_KR.md files. It incorporates details on how to use the library via CDN, updates links to external documentation, clarifies descriptions, and makes minor formatting adjustments. Additionally, it includes small refinements to the type definitions in data.d.ts to improve clarity and correctness regarding expected input types and add helpful comments.
Highlights
- CDN Installation Instructions: Added a new section in the READMEs detailing how to include the library using CDN links, providing an alternative to NPM installation.
- Link and Formatting Fixes: Updated links to external documentation (PixiJS and Pixi-Viewport) and fixed minor formatting issues like trailing commas in code examples.
- Type Definition Refinements: Adjusted type definitions for component textures and added clarifying comments for properties like
marginandassetin the data schema. - Package Description Update: Updated the package description in
package.jsonto be in English.
Changelog
Click here to see the changelog
- README.md
- Refined introductory text, changing 'pixijs' to 'pixi.js' and adding a note about understanding the base libraries (Diff 1, lines 4-6).
- Added a 'CDN' installation section with script tags (Diff 2, lines 54-58).
- Updated the Usage example, removing the async IIFE and adding a Codesandbox link (Diff 2, lines 61-99).
- Changed 'PixiJS' to 'pixi.js' in the
appoptions description (Diff 3, line 124). - Updated the link for
pixi-viewportdocumentation (Diff 4, line 137). - Fixed a trailing comma in the example data structure (Diff 5, lines 204-205).
- Changed 'pixiJS' to 'pixi.js' in the
assetsection (Diff 6, line 339).
- README_KR.md
- Refined introductory text, changing 'pixijs' to 'pixi.js' and adding a note about understanding the base libraries (in Korean) (Diff 1, lines 4-6).
- Added a 'CDN' installation section with script tags (in Korean) (Diff 2, lines 54-58).
- Updated the Usage example, removing the async IIFE and adding a Codesandbox link (in Korean) (Diff 2, lines 61-99).
- Changed 'PixiJS' to 'pixi.js' in the
appoptions description (in Korean) (Diff 3, line 125). - Updated the link for
pixi-viewportdocumentation (in Korean) (Diff 4, line 138). - Fixed a trailing comma in the example data structure (in Korean) (Diff 5, lines 204-205).
- Changed 'pixiJS' to 'pixi.js' in the
assetsection (in Korean) (Diff 6, line 339).
- package.json
- Updated the
descriptionfield from Korean to English (Diff 1, line 5).
- Updated the
- src/display/data-schema/data.d.ts
- Changed the type of
textureinBackgroundComponentfromstring | TextureStyletoTextureStyle(Diff 1, line 96). - Changed the type of
textureinBarComponentfromstring | TextureStyletoTextureStyleand added example formats for themarginproperty comment (Diff 2, lines 105, 108). - Added examples for the
assetproperty comment and example formats for themarginproperty comment inIconComponent(Diff 3, lines 121, 124). - Added example formats for the
marginproperty comment inTextComponent(Diff 4, line 136).
- Changed the type of
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Docs get a refresh,
CDN links now appear,
Code examples shine.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
MinCrohn
changed the title
There was a problem hiding this comment.
Code Review
This pull request brings several valuable improvements to the documentation, including the addition of CDN installation details, links to live examples, and consistent naming for pixi.js. The update to the package.json description and the enhanced comments in the type definitions are also welcome. The fixes to the example code snippets in the README files, ensuring they are syntactically correct, are particularly appreciated as they improve the usability of the examples.
However, there are a couple of points that need attention before merging, primarily concerning a potential breaking change in the type definitions and an inconsistency in the Korean README example.
Summary of Findings
- Potential Breaking Change in Type Definitions: The
textureproperty inBackgroundComponentandBarComponent(insrc/display/data-schema/data.d.ts) was changed fromstring | TextureStyletoTextureStyle. This could be a breaking change if string-based asset keys were previously supported for these components. - Inconsistent README Example: The Korean README (
README_KR.md) usestexture: 'loading'for an icon component in an example, while the English README and the type definitions useasset: 'loading'. This inconsistency should be resolved. - Documentation Improvements: The PR includes excellent documentation updates: CDN details, example links, consistent library naming (
pixi.js), and improved code examples. These are very positive changes. - Type Definition Comment Enhancements: The comments in
src/display/data-schema/data.d.tsformarginandassetproperties were improved with more examples, which enhances clarity. (Not commented on directly due to review settings focusing on medium+ severity issues).
Merge Readiness
The pull request introduces significant and valuable documentation enhancements. However, due to the high-severity concerns raised regarding a potential breaking API change in data.d.ts and an inconsistency in the README_KR.md example, I recommend that these issues be addressed before merging. Clarification on the texture type changes is particularly important. I am not authorized to approve pull requests, but based on this review, changes are requested.
| } | ||
| } | ||
| }, | ||
| { type: 'icon', texture: 'loading', size: 16 } |
There was a problem hiding this comment.
It seems there's an inconsistency in the icon component's property name here compared to the English README.md and the IconComponent type definition in src/display/data-schema/data.d.ts.
The type definition specifies asset: string; for icons, and the English README example uses asset: 'loading'. Using texture: 'loading' here might be misleading or incorrect based on the defined schema.
Could you update this to use asset for consistency and correctness?
| { type: 'icon', texture: 'loading', size: 16 } | |
| { type: 'icon', asset: 'loading', size: 16 } |
| type: 'background'; | ||
| show?: boolean; // default: true | ||
| texture: string | TextureStyle; | ||
| texture: TextureStyle; |
There was a problem hiding this comment.
The type for texture in BackgroundComponent has been changed from string | TextureStyle to TextureStyle. This implies that a simple string (e.g., an asset key) can no longer be used to define a background texture, and a TextureStyle object is always required.
If users were previously able to use a string here, this would be a breaking change. Could you clarify if this change is intentional and, if so, whether it's considered a breaking change? The README examples currently use the TextureStyle object, which aligns with this new type.
| type: 'bar'; | ||
| show?: boolean; // default: true | ||
| texture: string | TextureStyle; | ||
| texture: TextureStyle; |
There was a problem hiding this comment.
Similar to the BackgroundComponent, the type for texture in BarComponent has been changed from string | TextureStyle to TextureStyle. This also seems like it could be a breaking change if string-based texture definitions were previously supported for bars.
Could you please confirm the intent behind this change and its potential impact? Ensuring consistency with how textures are defined is good, but we should be clear about any API contract changes.
No description provided.