-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
feat(docs-infra): add stable to the list of api statuses #34981
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, @ajitsinghkaler! Can you also add some tests for the new feature?
@@ -117,7 +118,7 @@ export class ApiListComponent implements OnInit { | |||
const matchesQuery = (item: ApiItem) => | |||
sectionNameMatches || item.name.indexOf(query!) !== -1; | |||
const matchesStatus = (item: ApiItem) => | |||
status === 'all' || status === item.stability || (status === 'security-risk' && item.securityRisk); | |||
status === 'all' || status === item.stability || (status === 'security-risk' && item.securityRisk) || (status === item.stability && !item.securityRisk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last condition seems superfluous, since the earlier status === item.stability
would have matched already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the last one so that the security risk ones does not mix with the stable ones but you are right this condition is not right.
@gkalpak need your views on this should we add security risk items to the stable filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gkalpak and tests have already been added on line 82-85 it was just that the menu item needed to be added.
If we remove security risk from the stable API then the tests need to be updated otherwise these seem enough to me please have a look at the tests and tell me if any other tests are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, security risk is orthogonal to stability, so stable
should include securityRisk
items as well. I.e. the condition should remain: status === 'all' || status === item.stability || (status === 'security-risk' && item.securityRisk)
Fair enough about the tests. Ideally we should have e2e tests covering this feature, but we don't 😅.
(If you want to add some e2e tests, I won't stop you, but it isn't required for this PR 😁)
You can preview f013be0 at https://pr34981-f013be0.ngbuilds.io/. |
Earlier api can be filtered based on security risk and deprecated now added stable as a status for better user experience Fixes angular#30396
f013be0
to
722132c
Compare
@gkalpak made the changes I'll add e2e tests later in the weekend will make a new issue for keeping the progress status on it |
You can preview 722132c at https://pr34981-722132c.ngbuilds.io/. |
Earlier api can be filtered based on security risk and deprecated now added stable as a status for better user experience Fixes angular#30396 PR Close angular#34981
Earlier api can be filtered based on security risk and deprecated now added stable as a status for better user experience Fixes angular#30396 PR Close angular#34981
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Earlier api can be filtered based on security risk and deprecated now added stable as a status for better user experience
Fixes #30396
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Earlier only api can be filtered based on deprecated and security
Issue Number: #30396
What is the new behavior?
Api can be filtered using stable also
Does this PR introduce a breaking change?
Other information