-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Switching a schema from the Manage Schema #1191
Switching a schema from the Manage Schema #1191
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.
@A1O Thanks for the PR! I have a comment, once resolved, we could merge this in.
Do I need to add a unit test? |
Codecov Report
@@ Coverage Diff @@
## master #1191 +/- ##
=======================================
Coverage 93.41% 93.41%
=======================================
Files 113 113
Lines 4341 4341
=======================================
Hits 4055 4055
Misses 286 286
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
@A1O I'm re-evaluating the requirement, since having an anchor as the parent of the SchemaRow
component poses other issues.
- This leads to cases where we have to check if there is a closest button or another anchor, as you've done. There's no better way around this. Any other way of trying to achieve this will still end up as a workaround.
- We'll also have to handle keydown events separetely.
We cannot get rid of the anchor because we need it for better accessibility.
I understand the issue is specced to handle click on the entire row, but I think we would be better off making only the name of the schema as an anchor.
Could you please revert these changes and modify SchemaRow
and only update the schema name to be an anchor tag?
@pavish Yes, no problem. I have made the changes you requested. |
…985-switch-schema-from-manage-schema
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.
@A1O We have a dedicated milestone to improve the visual aspect of the design, where we intend to go through the entire product and improve design in general (UX + UI). While working on features, the priority is to focus more on the user experience side of the design. |
Fixes #985
Technical details
As described in the issue #985 description:
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin