Update required options for components #1979
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
October 7, 2020 16:24
The back-link text defaults to "Back". You can provide just a `href` to the component and it will render correctly. Therefore, mark `text` and `html` as not required.
The character count threshold triggers the count message to be hidden by default. If not set, the count message is visible all the time. Therefore, mark the count threshold as not required.
If checkbox item.name is not set, then the component global nameis used instead. Therefore, mark the checkbox item.name as not required.
Match the description for the `item.name` option - when ommitted, the global option is applied instead
If the date input label is not provided, the name is used instead. Therefore, mark the label as not required.
The visuallyHiddenText option was missing the required option. It has a default value of "Error" so is not required, but let's make that clear and keep things consistent.
36degrees
reviewed
Oct 8, 2020
5254cce to
bc91cc5
Compare
added 6 commits
October 8, 2020 15:50
The footer navigation title does not have a default. If navigation is provided, but the title attribute is missed, you end up with an empty heading tag. Therefore, mark the title as required.
Make it clearer that if items.id is not set, then `idPrefix` is required instead.
It is possible to pass navigation items without a `href` - you might want to do this if you're showing the current page. However, passing a navigation item without one of `text` or `html` results in no text or link rendering at all. Following the pattern we use for other components, we should mark `navigation.text` and `navigation.html` as required and use the description to explain that one overrides the other. The description already explains the relationship between the two. Therefore, we should mark the navigation.text and navigation.html as required.
Both `text` and `href` attributes need to be present to create a link. A navigation item with no href or text does not render - you end up with an empty <ul> element. We also instruct users that a navigation.title is required, so you also end up with a heading with an empty list underneath. Therefore, mark navigation.items.text and navigation.items.href as required
Passing a href by itsef results in an invisible link (no visible text). Passing text by itself results in a link with no href. Therefore, mark the footer meta.items.text and meta.items.href as required.
bc91cc5 to
bb983b7
Compare
36degrees
reviewed
Oct 8, 2020
| @@ -13,7 +13,7 @@ params: | |||
| description: Type of input control to render. Defaults to "text". | |||
| - name: inputmode | |||
| type: string | |||
| require: false | |||
| required: false | |||
There was a problem hiding this comment.
I love it when you improve your test coverage and it catches stuff like this! 👏🏻
Great job.
The way the previous tests were structured seemed to test for the presence of at least one single object within the component options with the required attributes, instead of all options having the correct attributes. The same was happening for the fixtures file. This meant that one missing `required` attribute in a component yaml file would not cause the test to fail. This commit tests for the presence of the required attributes by looping through each option/fixture. It also fixes one typo picked up by these new tests.
bb983b7 to
d52f62f
Compare
2 tasks
36degrees
approved these changes
Oct 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #1965
What
required: falserequiredattributes to optionsNot included:
required: falsebut failing to pass a tag means that a blank tag (with background colour) is rendered. See issue here.