Skip to content

[$25]Fix react-ui dependency and variable naming for Accessibility changes #3060

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

Closed
codeMinter opened this issue Jul 25, 2019 · 21 comments
Closed

Comments

@codeMinter
Copy link
Contributor

During the Accessibility we have used some variables in react-ui which are not in consistency with https://marvelapp.com/c7h2a6h/screen/59648056/handoff

Once the Accessibility Bug Bash is over, we will need to align this in react-ui and community-app repo to make them consistent and aligned to our handoff.

@veshu @sushilshinde @mishacucicea tagging you all so we don't miss this once we wind up Bug Bash.

@codeMinter
Copy link
Contributor Author

Fix this too - #2797 (comment)

Use the latest green color from GUI KIT.

@veshu
Copy link
Contributor

veshu commented Aug 8, 2019

We will fix this as the last issue

@veshu veshu added the On hold label Aug 8, 2019
@veshu veshu changed the title Fix react-ui dependency and variable naming for Accessibility changes [$25]Fix react-ui dependency and variable naming for Accessibility changes Aug 26, 2019
@veshu
Copy link
Contributor

veshu commented Aug 26, 2019

@topcoder-platform/topcodercompetitors This is open for pickup.

The fix is to remove the color variable not present on the handoff from https://github.com/topcoder-platform/topcoder-react-ui-kit and replace those variables used on the https://github.com/topcoder-platform/community-app and https://github.com/appirio-tech/accounts-app with respective allowed color variable.

e.g. $tc-dark-blue-120: #006ad7; in https://github.com/topcoder-platform/topcoder-react-ui-kit/blob/qa-accessibility/src/styles/_mixins/variables.scss is not allowed as handoff doesn't describe tc-dark-blue-120 so we need to remove this variable and replace this with tc-dark-blue-110 in community-app and accounts-app if that is used.

Another color variable to replace is '$tc-green-120: #328732;and use$tc-green-110: #328732`

Please make sure there is no other discrepancies on handoff guide and color variables.

Let us know if you need more info.

@nahidshahin nahidshahin self-assigned this Aug 26, 2019
@crazyk07
Copy link

Contest https://www.topcoder.com/challenges/30100341 has been created for this ticket.

This is an automated message for crazyk via Topcoder X

@crazyk07
Copy link

Contest https://www.topcoder.com/challenges/30100341 has been updated - the new changes has been updated for this ticket.

This is an automated message for crazyk via Topcoder X

@crazyk07
Copy link

Contest https://www.topcoder.com/challenges/30100341 has been updated - it has been assigned to nahidshahin.

This is an automated message for crazyk via Topcoder X

@crazyk07
Copy link

Contest https://www.topcoder.com/challenges/30100341 has been updated - it has been assigned to rohitgupta_.

This is an automated message for crazyk via Topcoder X

@r0hit-gupta
Copy link
Collaborator

@veshu as per the marvel handoff, the variable $tc-green-110 does not equals #328732. Can you please confirm what color to use? Thanks!

@veshu
Copy link
Contributor

veshu commented Aug 28, 2019

@veshu as per the marvel handoff, the variable $tc-green-110 does not equals #328732. Can you please confirm what color to use? Thanks!

@Dara-K Can you please help here? The color you have provided 328732 is not present on handoff. Whether we should update the palette or have $tc-green-120 variable as of now. Thanks!

@Dara-K
Copy link

Dara-K commented Aug 28, 2019

That's a really good question. When I updated the GUI KIT, I used the #258205 very dark green color that works for texts contrasts and passes accessibility test level AA2. Later, For the graphics elements, I tried to do a lighter version of green - #328732 which would still pass our accessibility tests, but not be that dark, as the colors are losing their contrasts between them and vibrance by darkening them so much, but I didn't add that to the GUI KIT.

To answer your question, based on the lightness of color, it should be:
tc-green-120: #258205
tc-green-110: #328732
But I am afraid this might mess with the other colors updates we did.

@r0hit-gupta
Copy link
Collaborator

@veshu how should we proceed here?

@veshu
Copy link
Contributor

veshu commented Aug 29, 2019

@Dara-K we are inter changing the values, so it won't affect the previous work we did, it just matching the variable with color shade you said above. @r0hit-gupta can you please confirm my understanding won't break the accessibility?

@r0hit-gupta
Copy link
Collaborator

@veshu you are right, interchanging the values shouldn't break anything.

@lakshmiathreya
Copy link

@veshu @r0hit-gupta @sushilshinde pls clarify how is this fix to be reviewed in QA?

@lakshmiathreya
Copy link

@drasticdpk @tosha5252 fyi

@lakshmiathreya lakshmiathreya added the Need clarification Need clarification to proceed fixing the issue further label Sep 3, 2019
@veshu
Copy link
Contributor

veshu commented Sep 4, 2019

@lakshmiathreya @drasticdpk @tosha5252 For this please verify that the accessibility is not broken for green color elements

@veshu veshu removed the Need clarification Need clarification to proceed fixing the issue further label Sep 4, 2019
@drasticdpk
Copy link
Collaborator

Hi @veshu @r0hit-gupta , here is few things which is break due to above changes , please update accordingly.
image

@drasticdpk drasticdpk added QA Fail QA verification on Dev has failed. Assignee to redo the fix. and removed Ready for QA labels Sep 5, 2019
@r0hit-gupta r0hit-gupta added tcx_ReadyForReview and removed QA Fail QA verification on Dev has failed. Assignee to redo the fix. labels Sep 6, 2019
sushilshinde added a commit that referenced this issue Sep 6, 2019
sushilshinde added a commit that referenced this issue Sep 6, 2019
@crazyk07
Copy link

crazyk07 commented Sep 9, 2019

Payment task has been updated: https://software.topcoder.com/review/actions/ViewProjectDetails?pid=30100341

This is an automated message for crazyk via Topcoder X

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants