search

cgkineo / adapt-pageNav

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

Issue/12 #13

Closed chris-steele closed 3 years ago

chris-steele commented 3 years ago

fixes #12

chris-steele commented 3 years ago

Addresses this change: https://github.com/adaptlearning/adapt_framework/blob/eccfcfd20d98f2efba5e08a752f46e1b548530b6/src/core/js/headings.js#L9 (v5.5.0)

moloko commented 3 years ago

@chris-steele does pageNav really need the screen reader completion heading? doesn't it a) complete immediately (thereby making it a bit pointless to tell a screen reader user that it's completed) and b) rarely have a title set?

moloko commented 3 years ago

@chris-steele does pageNav really need the screen reader completion heading? doesn't it a) complete immediately (thereby making it a bit pointless to tell a screen reader user that it's completed) and b) rarely have a title set?

I suppose it doesn't really matter, it should really be triggering the "view:render" event regardless.

@oliverfoster I'm assuming the reason it overrides AdaptView's render function is mainly because it needs do to var data = this.model.getData(); instead of the usual const data = this.model.toJSON(); - if that's the only reason could this perhaps be shifted to preRender to avoid the need to override render?

moloko commented 3 years ago

presumably it should also be amended to trigger "view:preRender" and "view:postRender" as well? (again, refactoring to avoid the override of render would make this comment irrelevant since AdaptView.render would then handle all of that)

chris-steele commented 3 years ago

@chris-steele does pageNav really need the screen reader completion heading? doesn't it a) complete immediately (thereby making it a bit pointless to tell a screen reader user that it's completed) and b) rarely have a title set?

The main issue is that you don't get the (accessible) heading at all because core/js/headings.js only renders when it receives the view:render event. I'd like to see the override of render go as well. Let's see what Mr @oliverfoster says.

oliverfoster commented 3 years ago

Sure. Dump this.model.getNavigationData() into a property on the model (_buttons or something) in preRender, drop the render function, remove the event trigger from postRender, and have the template read from _buttons instead.