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

Update xr-mode-ui.md #5420

Merged
merged 4 commits into from
Jan 12, 2024
Merged

Update xr-mode-ui.md #5420

merged 4 commits into from
Jan 12, 2024

Conversation

Omegahed
Copy link
Contributor

Description:

Corrected default values for enterVREnabled and enterAREnabled. Added examples to clarify customization options. Added "One of" column for greater clarity.

Changes proposed:

  • Update default values to match schema
  • Add code examples for greater clarity

@Omegahed
Update xr-mode-ui.md
c82aef8 
Corrected default values for enterVREnabled and enterAREnabled. Added examples to clarify customization options. Seperated out "one of" into seperate column for greater clarity.
dmarcos
dmarcos reviewed Dec 24, 2023
View reviewed changes
docs/components/xr-mode-ui.md Outdated
| enterAREnabled | If the AR button is displayed when applicable | false |
| XRMode | If the AR, VR button or both will be displayed. One of 'ar', 'vr' or 'xr'.| vr |
| Property | Description | Default Value | One of |
|-------------------------|---------------------------------------------------------------------|---------------|--------|
Copy link
Member

Choose a reason for hiding this comment

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

Adding a One Of column makes docs inconsistent with other components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Removed One Of column from pull request.

dmarcos
dmarcos reviewed Dec 24, 2023
View reviewed changes
docs/components/xr-mode-ui.md Outdated
| Property | Description | Default Value | One of |
|-------------------------|---------------------------------------------------------------------|---------------|--------|
| `cardboardModeEnabled` | Enables the now deprecated cardboard mode. | `false` | -
| `enabled` | Whether or not to display UI related to entering VR. | `true` | -
Copy link
Member

@dmarcos dmarcos Dec 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we don't use backticks in property names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed backticks ( ` ) from pull request.

dmarcos
dmarcos reviewed Dec 24, 2023
View reviewed changes
docs/components/xr-mode-ui.md Outdated
</a-scene>
```

### Specifying a Custom Enter AR Button
Copy link
Member

@dmarcos dmarcos Dec 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think these are a bit redundant. Can perhaps clarify the Specifying Custom Enter VR and Enter AR Buttons section if needed. I don't think we need three separate sections Specifying a Custom Enter VR Button, Specifying a Custom Enter AR Button and Specifying Custom Enter VR and Enter AR Buttons

dmarcos
dmarcos reviewed Dec 24, 2023
View reviewed changes
docs/components/xr-mode-ui.md Outdated
<a id="myEnterVRButton" href="#"></a>
<a id="myEnterARButton" href="#"></a>
</a-scene>
```
> ( i ) This is similar to the behavior of the `vr-mode-ui` component that the `xr-mode-ui` component replaced in A-Frame 1.5.0.
Copy link
Member

Choose a reason for hiding this comment

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

We use the deprecations section in release notes for this: https://github.com/aframevr/aframe/releases

Copy link
Member

Choose a reason for hiding this comment

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

Try to avoid info in docs info that loses relevance or gets obsolete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Removed the note from this section.

@dmarcos
Copy link
Member

dmarcos commented Dec 24, 2023

Thanks. I made a few comments. Some of the info I think is redundant. Covered in other places. If customizing the AR and VR buttons docs is not clear we can perhaps expand a bit while keeping it concise.

@dmarcos
Copy link
Member

dmarcos commented Dec 24, 2023 •
edited
Loading

We can perhaps just tweak the general description. Now

The xr-mode-ui component allows configuring and disabling of UI such as the enter VR / AR button, compatibility
modal, and orientation modal for mobile. The xr-mode-ui component applies only
to the a-scene. If we wish to simply toggle the UI, use CSS instead.

Alternative

The xr-mode-ui component configures or disables the enter VR and AR buttons. Buttons only display if the browser supports the corresponding modes (AR or VR). The xr-mode-ui component applies only
to the a-scene. If we wish to simply toggle the UI, use CSS instead.

Omegahed
Omegahed commented Jan 12, 2024
View reviewed changes
docs/components/xr-mode-ui.md
### Specifying Custom Enter VR and Enter AR Buttons

Display a custom Enter VR Button when VR is supported and a custom Enter AR button when AR is supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmarcos I've condensed the three sections back down to one. Hopefully, this is more concise and helps to clarify.

@dmarcos
Copy link
Member

dmarcos commented Jan 12, 2024

Thanks for the patience!

@dmarcos
Copy link
Member

dmarcos commented Jan 12, 2024

congrats on your first contribution!

@dmarcos dmarcos merged commit 298b1c9 into aframevr:master Jan 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmarcos dmarcos dmarcos left review comments

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
@Omegahed @dmarcos