-
Notifications
You must be signed in to change notification settings - Fork 47
feat: Add grey color for disabled selects during the form submission #716
Conversation
Thanks for the pull request, @Lunyachek! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
@Lunyachek there's a test failure, but it looks unrelated to this PR.
|
@revenue-squad this is an interesting case where CI is blocking the PR on security items that don't exist on master, but haven't been back-ported to the named release branch. In discussing this with my team we thought maintainers should probably be the ones to backport the security issues in cases like this. This repo isn't formally maintained, however. Thoughts on the best approach? |
@Lunyachek @e0d Greying-out of disabled items is appropriate UI but not something I caught because I assumed the page was short-lived at that point. No objection to doing so for a11y. But if we do that, I suggest using the Paragon colors for disabled items (which, oddly enough, are explicitly low-contrast); #ebebeb is 1.19:1 luminance contrast, which is in the right range. However, generally best to use the appropriate design tokens rather than magic color numbers. Paragon colors are here: https://github.com/openedx/paragon/blob/master/scss/core/_variables.scss |
@mphilbrick211 these tests currently can't pass unless we pull in some library upgrades from master, @pshiu was going to bring this topic up with the internal working group at 2U for their thoughts and then we can coordinate with BTR. |
Hi @pshiu any update on this? |
@mphilbrick211, I think we need to separate the remediation of the security vulnerability from this PR. @Lunyachek, I believe I do not have permissions to push to your fork, so could you please run |
Hi @Lunyachek - friendly ping on this :) |
Hi @Lunyachek - just checking in to see if you plan to pursue this pull request? If not, we will need to close it do to inactivity. Please let us know. Thanks! |
Hi @Lunyachek - final check in on this! CC @mariajgrimaldi - are we still planning to pursue this? |
Hi @mariajgrimaldi - do you think this is OK to close? I've marked it as inactive. We can reopen if author chooses to pursue. CC: @Lunyachek |
Hi @mariajgrimaldi - just checking on this. |
@mphilbrick211: yes, I agree. Thanks for the ping! |
@Lunyachek Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
This is the copy of PR to master branch #715
Description
This cosmetic enhancement for payment form. After pressing "Place Order" button - we can see that all inputs became disabled, but selects - no. It looks not consistent and we decided to fix this small issue.
2023-03-06.11.53.26.mov
The result:
2023-03-02.14.43.17.mov