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

Bring "Community components" example into A-Frame Examples. #5211

Merged
merged 14 commits into from
Mar 28, 2023

Conversation

diarmidmackenzie
Copy link
Contributor

Description:

1st PR for #5207. This updates the example intended to show use of community components.

A few notes on some choices I took here:

  • gradient-sky has been broken since 0.8.0, so I moved to a more current working sky component. There is a sun-sky component in superframe that I'd have liked to have used, but it's also not working at the moment. Hopefully we can repair that (along with the rest of superframe), and then maybe move to that. For now, this seems the best option.

  • I've standardized on jsdlivr rather than unpkg URLs. jsdelivr is more flexible as it can deliver resources from github as well as NPM, and seems to have more widespread usage in the community these days.

  • The docs have a long section explainign unpkg CDN. I'd like to rework this to cover multiple CDNs including jsdelivr, but I don't want to complicate this PR. So for now I have left the unpkg explanation as it is, and just added a brief intro to jsdelivr, so that the URLs in the example can be understood by a reader.

As mentioned in #5207, this is the 1st of 3 PRs to bring external content referenced in the documentation into the scope of the A-Frame Examples.

@diarmidmackenzie diarmidmackenzie marked this pull request as ready for review January 11, 2023 14:19
<script src="https://unpkg.com/[email protected]/dist/aframe-particle-system-component.min.js"></script>
<script src="https://unpkg.com/aframe-extras.ocean@%5E3.5.x/dist/aframe-extras.ocean.min.js"></script>
<script src="https://unpkg.com/[email protected]/dist/gradientsky.min.js"></script>
<title>Community Components Example</title>
Copy link
Member

Choose a reason for hiding this comment

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

We now have duplicated code here and the example that have to be in sync. It would be cool if there's a markdown way to display html from the file hosted on gihub

Copy link
Contributor Author

@diarmidmackenzie diarmidmackenzie Jan 13, 2023

Choose a reason for hiding this comment

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

One option:
https://github.com/zakhenry/embedme

A limitation is that code fragments are indexable only by line number. If line numbers change, extracts will get out of sync. That's discussed (together with some possible solutions) here but there doesn't seem to be a good solution yet (and momentum has stalled since 2020).

How widespread an issue do you think this is in the docs? Is there enough benefit to be worth the overhead of adding new tools like embedme to the pipeline? Is it worth doing if snippets have to be by line number, rather than tag-based as #48 will enable?

<script src="https://unpkg.com/[email protected]/dist/gradientsky.min.js"></script>
<title>Community Components Example</title>
<meta name="description" content="Community Components Example">
<script src="../../../dist/aframe-master.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

For embedded code in docs we can point to the A-Frame version. That gets updated automatically. If we point to dist/aframe-master once cannot copy / paste the code.

One of the features I once requested to glitch is having a glitch backed up by a git directory. These way we could have all the glitches in a single repo and update them all on a new release.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI for networked-aframe, I update the naf-examples glitch by opening the glitch terminal and git pull from github, see my procedure https://github.com/networked-aframe/networked-aframe/blob/master/docs/release-instructions.md
Each glitch is actually a git repository where it autocommit the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm proposing these lines in each example:

    <!-- If running this example without a local copy of A-Frame, replace this next line with:
    <script src="https://aframe.io/releases/<release_number>/aframe.min.js"></script>
    -->
    <script src="../../../dist/aframe-master.js"></script>   

Not perfect, but feels like a reasonable compromise?

Same applies to #5213, #5214, #5215.

Copy link
Member

@dmarcos dmarcos Mar 28, 2023

Choose a reason for hiding this comment

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

I think this embedded code should point to the latest A-Frame version as it was before. The docs update script updates the version automatically upon publication. Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, makes sense. Changed back to 1.4.0 version. Quoted code in the docs won't exactly match the code in the linked example, but the difference is hopefully not going to be too confusing to anyone...

@diarmidmackenzie
Copy link
Contributor Author

Moved from examples\showcase to examples\docs. Also fixed some bad links left over from #5215 where aincraft demo was moved from examples\showcase to examples\docs.

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Mar 27, 2023

Following up on this PR & also #5213

I believe both can be merged. Unresolved issues that came up in review were:

  • auto-sync to glitch. Technically complicated. Separate issue tracks the options here:
  • whether to refer to ../../../dist/aframe-master.js or aframe.io/releases/[release]/aframe.min.js - we agreed the best option is to refer to dist, with a comment indicating how to use aframe.io when copy/pasting the code. 5214 and 5215 already merged using this pattern. If we implement sync-to-glitch, it will be possible to adjust the code as part of the sync operation so that it works on glitch without a local aframe library.
  • desire to avoid duplication of code between documentation and example - unfortunately it doesn't look like any good options available here. As per this comment, embedme is promising, but will create maintenance pain as code snippets have to be referenced by line number, rather than by tags.

While it would be nice to solve some of the above issues, none is simple to solve, and I don't see a good case for keeping outdated example code in the meantime.

So on the basis that we merged #5124 and #5215 already, I think we should be OK to merge this one & #5213

@dmarcos
Copy link
Member

dmarcos commented Mar 28, 2023

Thanks for the patience. Commented. Just a nit.

Docs will be auto-updated using per-release process.
@dmarcos
Copy link
Member

dmarcos commented Mar 28, 2023

Super thanks!

@dmarcos dmarcos merged commit 84e766f into aframevr:master Mar 28, 2023
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.

3 participants