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

WIP: Enable error summary autofocus disable #1173

Closed
wants to merge 1 commit into from
Closed

WIP: Enable error summary autofocus disable #1173

wants to merge 1 commit into from

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Feb 5, 2019

any idea how to test this scenario in the app?

new ErrorSummary($errorSummary).init({ autofocus: false })

cc: @NickColley

@NickColley
Copy link
Contributor

NickColley commented Feb 5, 2019

I thought it might make more sense to be something like

new ErrorSummary($errorSummary, { autofocus: false }).init()

Where the second argument is stored on the instance as 'options'

@alex-ju
Copy link
Contributor

alex-ju commented Feb 5, 2019

We're not using options for any JavaScript modules at the moment, but rather rely on data- attributes. Would it be consistent to set data-autofocus="false" at the DOM level and pick that up via JS? This way we don't have to change anything in the way we initialise components.

@NickColley
Copy link
Contributor

NickColley commented Feb 5, 2019

I think bootstrap does a good job of having both with a data-api and programmatic api:

https://getbootstrap.com/docs/3.4/javascript/#js-data-attrs

So we could have a convention for components:

new ErrorSummary($errorSummary, { autofocus: false }).init()

or

<div data-module="error-summary" data-autofocus="false"></div>

@alex-ju what do you think? Maybe too over the top?

Seems like a good idea to go with the data-api first and then consider having a programmatic api when we have more components with options.

Only thing with data api is that we'd need to add them to the macros, whereas with the programmatic api we can transparently do this in the Design System...

@alex-ju
Copy link
Contributor

alex-ju commented Feb 5, 2019

@NickColley I'm all for having both programatically and data-api for all components and all options – I initially did the same with the character count (and other components), but while contributing it I ended up giving up on one of them as we were debating on which one should superseed the other – so we only kept the data-api version.

@36degrees
Copy link
Contributor

Yeah, I think even if we support data attributes we'll need some way to do this without changing the macros / HTML shown in the code examples.

@alex-ju
Copy link
Contributor

alex-ju commented Feb 5, 2019

@igloosi I think we went into styling instead of answering your question. what do you mean by testing? having a visual example or unit/functional test it?

@NickColley
Copy link
Contributor

NickColley commented Feb 5, 2019

You can test this by looking at document.activeElement which tells you which element is currently focused.

It'd require puppeteer style integration tests.

@36degrees
Copy link
Contributor

We have a test that the ErrorSummary is focussed already so that would be a sensible place to start.

@kr8n3r
Copy link
Author

kr8n3r commented Feb 5, 2019

in order to test both scenarios in the app, you need a way to have both versions of the error-summary running.

if that is data api driven, that's easy. for the programatic api test, we'd need a way to initialise the default and this examples separately.

atm, all examples are initialised in the app.js only

and disable autofocus if argument provided
@kr8n3r
Copy link
Author

kr8n3r commented Feb 5, 2019

code updated to reflect @NickColley's suggestion.
let me know about the decision to use data-api as well to control functionality

@hannalaakso hannalaakso added the awaiting triage Needs triaging by team label Feb 6, 2019
@NickColley
Copy link
Contributor

I did a quick spike to explore having a data api that mirrors options...

I'd be happy to put together a proper architecture proposal for this, not sure if it should block this feature.

https://jsbin.com/gumibix/14/edit?html,js,console,output

@kr8n3r
Copy link
Author

kr8n3r commented Feb 11, 2019

I'll close the PR until it's decided in the team then

@kr8n3r kr8n3r closed this Feb 11, 2019
@NickColley
Copy link
Contributor

Thanks Jani!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants