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: Added Navigation Button API #339

Merged
merged 29 commits into from
May 2, 2023
Merged

New: Added Navigation Button API #339

merged 29 commits into from
May 2, 2023

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Mar 30, 2023

fixes #338

New

  • NavigationButtonModel To hold the properties for each button
  • NavigationButtonView For extensions to make their own buttons and for legacy injected button management
  • NavigationModel To hold the navigation configuration
  • All model updates will be reflected in the DOM.

General API layout:

import navigation from 'core/js/navigation';
navigation.model.set('_showLabel', true); // Show all labels

const backButton = navigation.getButton('back');
backButton.set('_order', 400); // Move the back button to the right
backButton.set('text', 'New Label'); // Change the back button label text


navigation.removeButton(backButton); // Remove back button

// Remove all buttons
navigation.buttons.forEach(button => navigation.removeButton(button));

To make a home button:

image

import Adapt from 'core/js/adapt';
import navigation from 'core/js/navigation';
import NavigationButtonModel from 'core/js/models/NavigationButtonModel';
import NavigationButtonView from 'core/js/views/NavigationButtonView';

// Extend the model and view classes to add own behaviour

Adapt.on('navigation:ready', () => {
  const model = new NavigationButtonModel({
    _id: 'home',
    _order: 0,
    _event: 'homeButton',
    _showLabel: null,
    _classes: '',
    _iconClasses: 'icon-medal',
    _role: 'link',
    ariaLabel: 'Home',
    text: '{{ariaLabel}}'
  });
  const view = new NavigationButtonView({ model });
  navigation.addButton(view);
});

Each button has the potential for custom _id, _order, _event, _showLabel, _classes, _iconClasses, _role, ariaLabel and text, with global settings for _showLabel, _navigationAlignment and _isBottomOnTouchDevices.

_event has some default behaviour at "backButton", "homeButton", "parentButton", "skipNavigation" and "returnToStart".

Example:
Added visua11y and the above home button to the vanilla course
image

Backward compatibility

Older buttons should have an aria-label attribute or an .aria-label element in order for a text label to be automatically generated. A <span class="label" aria-hidden="true">{{ariaLabel}}</span> will be appended automatically to any existing button, if it doesn't exist, and will otherwise be automatically updated from the value of the aria-label attribute or .aria-label element. This is as model text defaults to {{ariaLabel}}. On older buttons "ariaLabel", "_role" and "_classes" have no effect when changed from the model as they should be supplied by the injected buttons themselves. "text", "_order", "_showLabel", "_id" and "_event" should work as expected on older injected buttons.

Requires

​https://github.com/adaptlearning/adapt-contrib-pageLevelProgress/pull/174
​https://github.com/adaptlearning/adapt-contrib-trickle/pull/188
​https://github.com/adaptlearning/adapt-contrib-vanilla/pull/415 - ✅ Merged

Testing

In the console it is possible to use:

  1. require('core/js/navigation').model.set('_showLabel', true);
  2. require('core/js/navigation').getButton('back').set('_order', 400);
  3. require('core/js/navigation').getButton('back').set('text', 'New Label');

@oliverfoster oliverfoster self-assigned this Mar 30, 2023
@oliverfoster oliverfoster added the enhancement New feature or request label Mar 30, 2023
@swashbuck
Copy link
Contributor

Created adaptlearning/adapt-contrib-pageLevelProgress#174 for Page Level Progress style changes.

@swashbuck
Copy link
Contributor

swashbuck commented Apr 4, 2023

@oliverfoster I've added a class called has-label-bottom that will move the labels below the icons. This should be an option at both the course level and the extension level.

Question: Are two text label position options enough? Default would be label on the right, and the new option would be label under the icon. Would it be better to have something like _labelPosition that can be set to top, left, right or bottom?

@oliverfoster
Copy link
Member Author

@oliverfoster I've added a class called has-label-bottom that will move the labels below the icons. This should be an option at both the course level and the extension level.

Question: Are two text label position options enough? Default would be label on the right, and the new option would be label under the icon. Would it be better to have something like _labelPosition that can be set to top, left, right or bottom?

I'm sure there will be an rtl requirement at minimum. @guywillis positioning controls at a course level?

@swashbuck
Copy link
Contributor

swashbuck commented Apr 5, 2023

I'm sure there will be an rtl requirement at minimum. @guywillis positioning controls at a course level?

@oliverfoster Ok, I can create classes for has-label-bottom, has-label-top, has-label-left, and has-label-right. That should cover everything. Then, we'll need a new setting/property for the model.

I think course-level positioning is probably fine?

@swashbuck
Copy link
Contributor

swashbuck commented Apr 11, 2023

Hide text below medium

I've implemented this in the Less for now. If we need to give devs more control (which might be overkill), we could add another course.json property.

Property name: _showLabelAtWidth

  • empty - Always show the label
  • small - Show at small width and up
  • medium - Show at medium width and up
  • large - Show at large width and up

This would add a class to .nav like show-label-small which can then be used to show the labels at certain breakpoints.

  "_navigation": {
    "_showLabel": true,
    "_showLabelAtWidth": "medium"
  }

@oliverfoster
Copy link
Member Author

Hide text below medium

I've implemented this in the Less for now. If we need to give devs more control (which might be overkill), we could add another course.json property.

Property name: _showLabelAtWidth

  • empty - Always show the label
  • small - Show at small width and up
  • medium - Show at medium width and up
  • large - Show at large width and up

This would add a class to .nav like show-label-small which can then be used to show the labels at certain breakpoints.

  "_navigation": {
    "_showLabel": true,
    "_showLabelAtWidth": "medium"
  }

I'd like to get @guywillis input on this bit as it was his biggest concern, along with a review of the styling.

@oliverfoster
Copy link
Member Author

I think, in lieu of an option in the JSON, these classes should be sufficient for custom styling.

What should be the default in either case?

@swashbuck
Copy link
Contributor

I think, in lieu of an option in the JSON, these classes should be sufficient for custom styling.

What should be the default in either case?

I think 'medium' is a good default. That's what we usually use in projects.

@swashbuck
Copy link
Contributor

swashbuck commented Apr 14, 2023

Positioning and breakpoint settings have been added at the top level.

course.json

  "_navigation": {
    "_showLabel": true,
    "_showLabelAtWidth": "small|medium|large", // default is medium
    "_labelPosition": "left|right|top|bottom" // default is right
  },

Schemas will need to be updated.

@oliverfoster oliverfoster marked this pull request as ready for review April 17, 2023 16:01
@swashbuck
Copy link
Contributor

Schemas have been added for the new options.

@swashbuck
Copy link
Contributor

One final comment to raise from myself. The setting _showLabelAtWidth assumes the course creator will always want to hide the labels at a certain adapt breakpoint - what if I want to always show the labels?

@guywillis I've modified the styles a bit and added a new option called "any" that will show the labels at any size. Labels can still be hidden completely at the global level with _navigation._showLabel: false or at the plugin level (ex. PLP adaptlearning/adapt-contrib-pageLevelProgress#180 ).

Copy link
Contributor

@guywillis guywillis left a comment

Choose a reason for hiding this comment

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

👍

Couple of comments to adjust defaults but otherwise I'm happy - really nice work

js/models/NavigationModel.js Outdated Show resolved Hide resolved
js/views/navigationView.js Outdated Show resolved Hide resolved
Copy link
Contributor

@swashbuck swashbuck left a comment

Choose a reason for hiding this comment

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

👍

@oliverfoster
Copy link
Member Author

One more approval and we're ready to merge.

@oliverfoster oliverfoster merged commit d64f999 into master May 2, 2023
@oliverfoster oliverfoster deleted the issue/338 branch May 2, 2023 08:42
github-actions bot pushed a commit that referenced this pull request May 2, 2023
# [6.34.0](v6.33.0...v6.34.0) (2023-05-02)

### New

* Added Navigation Button API (#339) ([d64f999](d64f999)), closes [#339](#339)
@oliverfoster
Copy link
Member Author

🎉 This PR is included in version 6.34.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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 this pull request may close these issues.

Navigation Button API
5 participants