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

Documentation for AssetPaths #42

Merged
merged 6 commits into from
Mar 6, 2023
Merged

Documentation for AssetPaths #42

merged 6 commits into from
Mar 6, 2023

Conversation

T1mL3arn
Copy link
Contributor

To address this HaxeFlixel/flixel-docs#265

Geokureli
Geokureli requested changes Feb 21, 2023
View reviewed changes
Copy link
Member

@Geokureli Geokureli left a comment

Choose a reason for hiding this comment

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

Pretty good, just a couple notes

default/source/AssetPaths.hx Outdated Show resolved Hide resolved
default/source/AssetPaths.hx Outdated Show resolved Hide resolved
default/source/AssetPaths.hx Outdated Show resolved Hide resolved
T1mL3arn added 2 commits March 3, 2023 14:41
@T1mL3arn
address review issues
058f5ed 
remove "more control" (will be in other doc)
add @see tags
@T1mL3arn
a note about where is this class from
92743a4
T1mL3arn added a commit to T1mL3arn/flixel that referenced this pull request Mar 3, 2023
@T1mL3arn
move some doc from AssetPaths
cdce831 
as it was suggested in HaxeFlixel/flixel-templates#42 (comment)
@T1mL3arn T1mL3arn mentioned this pull request Mar 3, 2023
Merged
@Geokureli
Copy link
Member

I made some changes:

  • added the left margin of * chars, this is standard practice for flixel comments
  • vscode has trouble showing markdown when there's ```haxe sections in there. sometimes it works, this time it didn't (see image below)
  • I reworded some things, let me know if you have any issues with this

Screenshot 2023-03-03 at 1 43 36 PM

@T1mL3arn
Copy link
Contributor Author

T1mL3arn commented Mar 4, 2023

It was supposed to manually add * at the very very end of the pr. I remember flixel repo has formatter set up, but I am not sure it supports autoformatting comments. Does it?

I have no such problem with markdown rendering (VSCode is v1.74.1, vshaxe 2.26.0) inside files (left version without * colors bold list items differently though).
image

Also hovercard hint works properly
image

Having full code examples is more expressive since the examples are tested and thus implies less quirks and less questions "how would I do that", so I strongly believe they should be left in the doc (e.g. FlxSprite have a lot of such examples). If we agreed with this I will revert them back.

Geokureli added a commit to HaxeFlixel/flixel that referenced this pull request Mar 6, 2023
@T1mL3arn @Geokureli
Move doc from AssetPaths to FlxAssets (#2747)
c3dba06 
* move some doc from AssetPaths

as it was suggested in HaxeFlixel/flixel-templates#42 (comment)

* add left *

* put back spaces

* clean up rename example

* fix md issues

* remove non-existent link

---------

Co-authored-by: George FunBook <[email protected]>
@Geokureli
Copy link
Member

Geokureli commented Mar 6, 2023 •
edited
Loading

I have no such problem with markdown rendering (VSCode is v1.74.1, vshaxe 2.26.0) inside files (left version without * colors bold list items differently though).

I can see in both your images that there are problems with the @see tags

Having full code examples is more expressive

I disagree, I don't think they add much and may even add confusion. this is fine and shorter

@Geokureli Geokureli merged commit 2290b3c into HaxeFlixel:dev Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Geokureli Geokureli Geokureli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@T1mL3arn @Geokureli