liferay / liferay-ckeditor

Other
8 stars 49 forks source link

fix: improve accessibility of caption styles #140

Closed robertoDiaz closed 3 years ago

wincent commented 3 years ago

Assigned this one to @julien seeing as you two already discussed this over on LPS-123682.

@robertoDiaz, I retitled the PR for you to start with fix: because the Semantic Pull Request bot will complain about anything that doesn't follow the Conventional Commits format. Would you mind git commit --amend-ing the commit and force-pushing to update the PR to make the bot happy (it'll only turn green when both PR and commit titles follow the format)?

julien commented 3 years ago

thanks @wincent.

@marcoscv-work, @eduardoallegrini could you tell me what you think about these changes, and which way we should fix it when you have time? thanks

marcoscv-work commented 3 years ago

This PR would "fix the issue" only in the edit view, so we think we need to fix it in all places/situations, that's the reason we have created new issues in Clay and a request for the Lexicon team:

We need to discuss with Lexicon what is the correct place for table captions (top or bottom) and maybe think about the possibility to change the edit textarea bg color.

So guys, can we close this PR and try to fix the root problem (no definition in Lexicon/Clay)?

marcoscv-work commented 3 years ago

/cc @victorvalle ;-) !

julien commented 3 years ago

@marcoscv-work i don't mind closing it, but I just want to make sure we don't forget about @robertoDiaz :)

marcoscv-work commented 3 years ago

@marcoscv-work i don't mind closing it, but I just want to make sure we don't forget about @robertoDiaz :)

Yeah! of course! It would be perfect if we could prioritize this since it is coming from a support bug. And actually, the bg-gray in the editor is a potential hive of contrast accessibility bugs. Since what you are editing in ckeditor is conceptualized to be finally rendered in a white bg (if we want to ensure accessibility).

julien commented 3 years ago

assigned to @marcoscv-work since he'll be working on this with @victorvalle

robertoDiaz commented 3 years ago

Hey @marcoscv-work

@wincent suggested me to change the commit message, but if I understand it well this code is going to be discarded.

Do I have to change the commit message?

wincent commented 3 years ago

@robertoDiaz: Don't bother changing the commit message just yet. Let's wait until we hear confirmation from @marcoscv-work that he's actually going to take over — his last comment ("It would be perfect if we could prioritize this") makes it sound like he is waiting for a green light (if so, from where?). Once we have clarity on that we can decide whether to close this or proceed with it (at which point, yes, we would have to amend the commit message in order to get a green CI run).

robertoDiaz commented 3 years ago

Hey @marcoscv-work

What is the current status of this?

Thanks!!

pat270 commented 3 years ago

This should be fixed by https://github.com/liferay/clay/pull/3822. Can we close this pr?

wincent commented 3 years ago

This should be fixed by liferay/clay#3822. Can we close this pr?

Thanks @pat270. I'm going to update the LPS with a link to your alternative fix.