ResponsePatterns should be a class

phetsims / utterance-queue

Alerting library powered by aria-live

MIT License

0 stars 2 forks source link

ResponsePatterns should be a class #32

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

(renamed from VoicingResponesPatterns in https://github.com/phetsims/utterance-queue/issues/31).

jessegreenberg commented 2 years ago

Im sure this is great, but can you please comment on why it is beneficial to be able to instantiate a ResponsePatterns?

zepumph commented 2 years ago

Only minor reasons as I worked on https://github.com/phetsims/utterance-queue/issues/31.

I noticed that the type def in Voicing.setVoicingResponsePatterns takes only an object.
I see that the primary way of creating response patterns is with ResponsePatterns.createResponsePatterns(). So to me the main question is really, "why is this factory-ish method better than just using a POJO, when we then need validation like validatePatternKeys. A POJO will make this more type safe (especially with typescript coming soon).

zepumph commented 2 years ago

Another minor thing. @jessegreenberg do you think we should rename to something that is singular?

zepumph commented 2 years ago

@jessegreenberg please review, and comment on the above thought about a rename.

jessegreenberg commented 2 years ago

The change looks good. A rename would also make sense. Was originally plural because the VoicingResponsePatterns object was expected to contain multiple objects like DEFAULT_RESPONSE_PATTERNS that might be helpful. Now that it is a class the reusable the reusable ones can be added statically like

ResponsePattern.MODIFIED_RESPONSE_PATTERNS = new ResponsePattern( {
  //...
} )

It will contain a collection of response patterns, so maybe the name should be ResponsePatternCollection? With usages like

this.voicingResponsePatterns = ResponsePatternCollection.MODIFIED_RESPONSE_PATTERNS

this.voicingResponsePatterns = new ResponsePatternCollection( {
  nameContextHint: '{{NAME}}! {{CONTEXT}}? {{HINT}}...',

zepumph commented 2 years ago

Hows that?

jessegreenberg commented 2 years ago

Looks very good, thanks for this improvement! I like how this looks when overriding the default patterns

      voicingResponsePatternCollection: new ResponsePatternCollection( {
        nameObjectHint: '{{OBJECT}}, {{NAME}}, {{HINT}}',
        nameObject: '{{OBJECT}}, {{NAME}}'
      } )