Accessibility in the statistics table#3380
Accessibility in the statistics table#3380Andrea-Guevara wants to merge 17 commits intoDSpace:mainfrom
Conversation
tdonohue
added
bug
accessibility
component: statistics
1 APPROVAL
|
@Andrea-Guevara : This is failing e2e tests (run via I think you just need to update each of the failing |
|
Good morning @tdonohue! I confess that I still need to improve a lot in relation to unit tests. At first I went back to the tags that were being used. As I did this implementation using localhost, and I don't have geoIp configured to get a more detailed report on the statistics; I would like to ask that the person who is going to test this implementation try to do it in a more complete way, with more than one statistics table if possible. This is to test the dynamic behavior of the new label. |
|
@Andrea-Guevara : I think you may have misunderstood my feedback. You are welcome to change the HTML tags if you need to do so. However, sometimes if you change a tag, you will have to also change it in tests. Some of our tests reference specific HTML tags, so if you change the HTML tag, then you have to also update the test to reference the new name. In the case of this PR, it looks like we have several e2e tests named So, if you wanted to change that I hope that makes sense that you are allowed to change the HTML tags if you need to do so. But, when you do so, if you find tests that fail, that is an indication that you also must update the tests. I haven't had a chance to give this PR a test/detailed look. But, you are welcome to still consider changing these HTML tags if you feel you need to. |
…s for the getObjectHeaderLabel method
|
Hi @Andrea-Guevara, |
|
Hi @Andrea-Guevara, |
tdonohue
added
port to dspace-7_x
|
Hi @Andrea-Guevara, |
tdonohue
left a comment
tdonohue
left a comment
There was a problem hiding this comment.
@Andrea-Guevara : I gave this PR a code review today. Overall, the code looks good, but I have a minor suggestion below. Also, this currently has Merge Conflicts, so it's not in a testable state. Could you resolve those merge conflicts please?
| getObjectHeaderLabel(reportType: string): string { | ||
| switch (reportType) { | ||
| case 'TotalVisits': | ||
| return this.translateService.instant('statistics.table.header.TotalVisits'); |
There was a problem hiding this comment.
I think you can get rid of this entire switch if you just append the reportType similar to this:
return this.translateService.instant('statistics.table.header.' + reportType);
That way, if new reports are added, we don't need to add more cases to this switch.
|
Good afternoon @tdonohue! Sorry for the delay in replying, I hadn't seen your message. I've resolved the conflicts and refactored the code according to your suggestion. I really appreciate your suggestions for improving the pulls. I've learned a lot from you over the last few days. Thank you very much. The test is giving an error, but it seems that some configuration is different. |
| }); | ||
|
|
||
| it('should return an empty string for unknown report types', () => { | ||
| expect(component.getObjectHeaderLabel('UnknownType')).toEqual(''); |
There was a problem hiding this comment.
This is where your tests are failing. It looks like you previously were expecting getObjectHeaderLabel() to return an empty string if you passed it an unrecognized type.
This isn't going to work the same anymore, because you are always just returning statistics.table.header.[type] in the code in getObjectHeaderLabel(). So, I think you can just remove this test... all types will now be looked up in the i18n files. This seems appropriate though because it allows for adding new report types without having to modify your code.
|
Hi @Andrea-Guevara, |
|
Hi @Andrea-Guevara, |
|
Closing, replaced by #4661 |
github-project-automation
bot
moved this from 👀 Under Review
to ✅ Done
in DSpace 10.0 Release
2 participants
References
Description
Creation of a method that conditionally defines a dynamic label for the object column.
Instructions for Reviewers
List of changes in this PR:
Include guidance for how to test or review your PR.
Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.