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

Update and modernize code #41

Closed
swashbuck opened this issue Feb 10, 2023 · 7 comments · Fixed by #49
Closed

Update and modernize code #41

swashbuck opened this issue Feb 10, 2023 · 7 comments · Fixed by #49
Assignees
Labels
enhancement New feature or request released

Comments

@swashbuck
Copy link
Contributor

swashbuck commented Feb 10, 2023

Subject of the issue

The Page Nav plugin codebase needs to be updated / modernized. This would include:

  • ES6 syntax
  • JSX template
  • Update documentation as needed

In addition, it has been mentioned that there is too many config options and that the plugin config could be streamlined. Look into decluttering the interface / setup.

@swashbuck swashbuck added the enhancement New feature or request label Feb 10, 2023
@swashbuck swashbuck changed the title Convert templates to JSX and update JS to ES6 syntax Update and modernize codebase Aug 30, 2023
@swashbuck swashbuck changed the title Update and modernize codebase Update and modernize code Aug 30, 2023
@swashbuck swashbuck self-assigned this Aug 30, 2023
@swashbuck
Copy link
Contributor Author

swashbuck commented Aug 30, 2023

@guywillis @oliverfoster I would like to work on this ticket. Before I get too deep into it, I would like to get your input first, mainly on the config interface decluttering.

Did either of you have specific things in mind that could be improved? Some suggestions I have:

  1. "Align icon right"
    a. This could be changed to a dropdown for "Icon alignment" / _iconAlignment with options for left, right, and auto (detects if using RTL or LTR). Defaults to "auto".
    b. Alternatively, we could remove this option and base the position on RTL/LTR and the type of button (ex. previous or next). We would almost always want the previous button's icon to be left-aligned (or right-aligned for RTL), so not sure that we need to expose it as an option.
  2. "Icon class" - Should the "Icon class" default to an appropriate icon in the schemas? It may be easier for course creators to remove icon classes as opposed to looking up the correct ones to use?
  3. "Override the route id" - Is this option needed Sibling buttons? Not needed, will remove
  4. "Sibling buttons"
    a. Rename to something like "Numbered paging buttons"
    b. Do we need to be able to set the "Text" value or can it just always display a number?
    c. Do we need _iconClass since we recommend no icon?
  5. Return to previous location - Do we need this at all? I find this option confusing and we never use it. If we keep it, it needs a description in the AAT.
  6. Lock until page complete - Do we need this option? We typically use Trickle.
  7. Is Continuous / _loopStyle - Still needed?
  8. Classes for each button - Do we need this in addition to _iconClass? Or is _iconClass enough?
  9. Close - Should this include all the functionality from adapt-close including the _closeViaLMSFinish and _notifyPromptIfIncomplete options?

Happy to set up a meeting to discuss. Thanks!

@oliverfoster
Copy link
Member

oliverfoster commented Aug 31, 2023

  1. "Override the route id" is probably not needed on the sibling buttons
  2. b. It's difficult to map the number of siblings to the number of override text labels as "siblings" is just an abstract undefined number thereof, unless the override for page nav text labels lives on the sibling as a page nav specific alternative title, which is very messy - also not sure if useful

@swashbuck
Copy link
Contributor Author

4b. It's difficult to map the number of siblings to the number of override text labels as "siblings" is just an abstract undefined number thereof, unless the override for page nav text labels lives on the sibling as a page nav specific alternative title, which is very messy - also not sure if useful

@oliverfoster The only use case I can think of would be to add a pound sign before each number. However, that doesn't really add much value imo. I would be in favor of removing the text option for sibling buttons altogether.

#{{inc index}}

@swashbuck
Copy link
Contributor Author

I'm at a stopping point with PR #49 until we resolve the questions above.

@oliverfoster
Copy link
Member

oliverfoster commented Sep 11, 2023

  1. Icon alignment (left/right/top/bottom/auto)
  2. Yes to default
  3. n/a
  4. Remove siblings from pageNav
  5. Remove _returnToPreviousLocation
  6. Keep lock until page complete
  7. Keep looping
  8. Leave in _classes
  9. Yes, make it consistent with adapt-close

@swashbuck
Copy link
Contributor Author

  1. Yes, make it consistent with adapt-close

I've moved this one to #50

@danielghost danielghost added the question Further information is requested label Nov 14, 2023
github-actions bot pushed a commit that referenced this issue Feb 27, 2024
# [3.0.0](v2.4.0...v3.0.0) (2024-02-27)

### Breaking

* Update and modernize, simplify configuration (fixes #41 #48) ([2663645](2663645)), closes [#41](#41) [#48](#48)

### onButtonClick

* Lookup model props instead of checking for CSS classes ([a3cb053](a3cb053))
Copy link

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@swashbuck swashbuck removed the question Further information is requested label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants