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

Add viewport-presets #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

omarbenmegdoul
Copy link

Make sure that your submission has the following:

  • A link to the raw github markdown file with a YAML front matter definition containing:
    • The title of your extension
    • A description of your extension
    • The author
    • A hero image
    • An icon
    • Tags (these will help with your extension's discoverability)
    • A link to your extensions manifest

Please, go through these steps before you submit a PR.

  • You have done your changes in a separate branch.

  • You have a descriptive commit message with a short title (first line).

  • You have only one commit (if not, squash them into one commit).

  • Your pull request MUST target the main branch on this repository.

  • Your pull request puts your extensions details as the last entry in the extensions.json file

@nthouliss
Copy link
Contributor

The other tag should only be used if you don't believe that your extension can fall into the other categories. I'd remove it and just have tool.

@omarbenmegdoul
Copy link
Author

Fixed conflicts and updated the extension, ty @nthouliss for the review

@Several-Record7234
Copy link
Collaborator

Validation script and image URLs check out fine. Extension functionality and API usage not checked.

@nthouliss
Copy link
Contributor

Thanks @omarbenmegdoul for the tag change.

At the moment you're triggering an unsubscribe and re-subscribe to the Party on every user input (like typing the name of the view or clicking the help button). You need to make sure to add your dependencies on your useEffect calls:

useEffect(
  () =>
    OBR.party.onChange((party) => {
      // React to party changes
    }),
  [] <-- Make sure this is here
);

The only other things I would look at is changing you help docs to say Reset rather than Reset View here:

Use the "Reset View" button to move your viewport back to the scene default

There's also a bit of an issue with Player Images. When I have multiple players and hit Player Images and their tokens are far from each other you end up between them. I'd expect that I could toggle through them but something even better would be if this was a list or another page where I could select a specific player character token. You're not required to resolve this to submit but it would help with UX.

I'd also change the text from OK to SAVE for saving the view preset to make it a little more obvious but that's just an opinion and not something required for submission.

I'd also look into having your toggles to turn off player created views in another page as a list (maybe even with the Player Images button). Again this isn't required for submission, just a suggestion.

If you need or want to do some in-app routing then react-router is a good one:

https://reactrouter.com/en/main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants