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

New: Add support for Tooltip API and Navigation Button API (fixes #16 #23) #24

Merged
merged 12 commits into from
Aug 14, 2023

Conversation

swashbuck
Copy link
Contributor

@swashbuck swashbuck commented Jul 19, 2023

Fixes #16 #23

Fix

  • Adds support for Tooltip and Navigation Button APIs
  • Allows overrides at the menu and content object level
  • Creates JSX template for button contents

Testing

Configure a course per example.json

To test overrides, add the config to a menu or content object. Then, simply use a non-empty value for the text that should be overridden (ex. _homeButton.navLabel or _homeButton.alt). In the AAT, these values should default to empty, so adding any text will override them.

To do

  • Finish updating schemas
  • Update readme

@swashbuck swashbuck self-assigned this Jul 19, 2023
@swashbuck swashbuck changed the title Fix: Add support for Tooltip API and Navigation Button API (fixes #16 #23) New: Add support for Tooltip API and Navigation Button API (fixes #16 #23) Aug 1, 2023
@swashbuck swashbuck marked this pull request as ready for review August 1, 2023 14:58
Copy link
Contributor

@danielghost danielghost left a comment

Choose a reason for hiding this comment

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

Updated code in 3466fc7 as part of review rather than leaving suggestions.

If the button can be overriden at a contentObject level as per the original issue, the schema will need updating to reflect this. If including the ability to use different labels, should this be extended to _classes and _iconClasses too? Is homeButton an appropriate name if it can be redirected any _id?

@swashbuck
Copy link
Contributor Author

swashbuck commented Aug 4, 2023

Thanks for the changes, @danielghost

If the button can be overriden at a contentObject level as per the original issue, the schema will need updating to reflect this.

Yep, agreed. The schema updates are still pending. I just wanted to nail down the data structure first. You can ignore the schema changes that are currently attached to this PR.

If including the ability to use different labels, should this be extended to _classes and _iconClasses too?

I don't know that this is necessary? The button's class is essentially determined by the _hideHomeButton and _hideBackButton settings. Seems like this might create a lot of rework, if nothing else. I would like to get the opinion of those who have more history with the development of this component, though.

Is homeButton an appropriate name if it can be redirected any _id?

This is a good point, but I feel that it would be out of scope for this ticket and better handled in a new one? It would definitely be a breaking change. Again, though, I would like to hear what others think.

@swashbuck swashbuck requested a review from guywillis August 4, 2023 21:13
@oliverfoster oliverfoster merged commit 04fbc9e into master Aug 14, 2023
@oliverfoster oliverfoster deleted the issue/16 branch August 14, 2023 14:28
@github-actions
Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@swashbuck swashbuck mentioned this pull request Aug 14, 2023
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.

Add support for Tooltip API and Navigation Button API
3 participants