cgkineo / adapt-pageNav

Navigation bar component (quicknav clone)
GNU General Public License v3.0
6 stars 4 forks source link

Update and modernize code #41

Closed swashbuck closed 6 months ago

swashbuck commented 1 year ago

Subject of the issue

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

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 commented 1 year ago

@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 commented 1 year ago
  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 commented 1 year ago

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 commented 1 year ago

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

oliverfoster commented 1 year ago
  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 commented 1 year ago
  1. Yes, make it consistent with adapt-close

I've moved this one to #50

github-actions[bot] commented 6 months ago

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: