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

Fix: Title is required to set notify popup dialog label (fixes #294) #295

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

kirsty-hames
Copy link
Contributor

Fixes #294

Fix

  • Set title as required. Without a title, the popup dialog won't be properly identified by assistive technologies.

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.

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@swashbuck
Copy link
Contributor

@kirsty-hames Does this need fixed in component.schema.json as well?

@kirsty-hames
Copy link
Contributor Author

@kirsty-hames Does this need fixed in component.schema.json as well?

Thanks @swashbuck. I wasn't too sure about this as there's a lot of inconsistencies between required properties across component.schema.json and properties.schema. For example, there are 18 properties set to "required": true in properties.schema, but only _graphic.src is set as required in component.schema.json..

Is this just an oversight? If so I'll raise a separate issue and just set title as required in this PR.

@swashbuck
Copy link
Contributor

@joe-allen-89 @taylortom Any thoughts on Kirsty's question above? What are the guidelines for using "required" true vs. false?

@joe-allen-89
Copy link
Contributor

joe-allen-89 commented Feb 12, 2024

@kirsty-hames @taylortom @swashbuck This looks to be an oversight, all the schemas were set up using adapt-octopus which seems to factor in whether the attribute has a default value for some reason before including it in the new schema - https://github.com/adapt-security/adapt-octopus/blob/d7c31730567ecbc01cbbb9ee6caa5ae534d95df7/lib/SchemaNode.js#L184

I'm just looking at a few other schemas to double check but this is likely to be an issue across all repos as they were all set up in the same way.

@taylortom
Copy link
Member

taylortom commented Feb 12, 2024

This one isn't an oversight actually, it's as designed.

In our case, defining an attribute as required while also having a default value is unnecessary, as the value will be set to the default (although the JSON schema allows anything to be used as a default value, even something that doesn't itself pass the schema valuation - but there's no reason for us to do this, or indeed expect it).

Therefore for brevity, we decided to remove the required definition if there's a default present. That being said, there's no problem with re-adding required definitions if there's a good reason though.


It's also worth noting that the required notation has changed with the new JSON schemas, see:
https://json-schema.org/learn/getting-started-step-by-step#define-required-properties

Summary

No longer a boolean, now an array:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "$id": "https://example.com/product.schema.json",
  "title": "Product",
  "description": "A product from Acme's catalog",
  "type": "object",
  "properties": {
    "productId": {
      "description": "The unique identifier for a product",
      "type": "integer"
    },
    "productName": {
      "description": "Name of the product",
      "type": "string"
    },
    "price": {
      "description": "The price of the product",
      "type": "number",
      "exclusiveMinimum": 0
    }
  },
  "required": [ "productId", "productName", "price" ]
}

@joe-allen-89
Copy link
Contributor

@taylortom I'd misunderstood that by setting a default forms expects a value, thinking the default could be changed to blank and potentially causing an issue.

As there's no defualt value for title, we'll either need to add a default value or add it as required.

@taylortom
Copy link
Member

taylortom commented Feb 13, 2024

@joe-allen-89 Ah yeah I think that's how legacy works actually, as there's no server validation.

With v1, you'll get the default from both the UI (when the form's rendered) and the API server (when the data is saved), so if you remove the default value that's been populated for you in the form, it should be re-added again by the server API when the validation runs.

The required property then sits on top of everything else and returns an error if no value has been provided for that field.

@kirsty-hames
Copy link
Contributor Author

This PR will need testing to confirm the latest schema commit a5122c8. @joe-allen-89 / @taylortom, are you able to confirm please?

@joe-allen-89 joe-allen-89 merged commit eacdcd8 into master Mar 4, 2024
github-actions bot pushed a commit that referenced this pull request Mar 4, 2024
## [6.11.1](v6.11.0...v6.11.1) (2024-03-04)

### Fix

* Title is required to set notify popup dialog label (fixes #294) (#295) ([eacdcd8](eacdcd8)), closes [#294](#294) [#295](#295)

### Upgrade

* Bump ip from 1.1.8 to 1.1.9 (#296) ([e8305ca](e8305ca)), closes [#296](#296)
@joe-allen-89 joe-allen-89 deleted the issue/294 branch March 4, 2024 10:32
Copy link

github-actions bot commented Mar 4, 2024

🎉 This PR is included in version 6.11.1 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Title is required to set notify popup dialog label
7 participants