Skip to content
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

#333: Replaced placeholder with label according to accessibility guid… #455

Merged
16 commits merged into from
May 24, 2022

Conversation

KattisLej
Copy link
Contributor

@KattisLej KattisLej commented May 17, 2022

Related issue(s) and PR(s)

This PR closes #333 .

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

List of changes made

Placeholder attribute are not accessible instead there's a recommendation to create a descriptive label instead of the placeholder.

Screenshot of the fix

image

Definition of Done checklist

  • I have made an effort making the commit history understandable
  • I have performed a self-review of my own code and commented any hard-to-understand areas
  • Tests and lint/format validations are passing
  • My changes generate no new warnings

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding labels makes the gui easier to understand, but some more changes are needed to avoid that the labels pushes the elements out of sight.

@jonandernovella jonandernovella self-requested a review May 19, 2022 11:17
@HenrikeW HenrikeW mentioned this pull request May 19, 2022
8 tasks
@KattisLej KattisLej added frontend This is likely affecting the frontend accessibility labels May 20, 2022
@KattisLej KattisLej requested review from a user and removed request for jonandernovella May 20, 2022 16:17
Copy link
Contributor

@HenrikeW HenrikeW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, much better now with the labels and the save button that doesn't disappear when zooming 💯
I'd do some slight change for aesthetic reasons, I think the whole footer should have a bit more space to breathe in the top (mostly visible above the "Add new issue" heading and the "You have unsaved changes" notice). Easiest probably is to give the .footer-container more top padding.
Also one comment in the code about a similar thing.

And a side note: I personally dislike the bootstrap margin classes because they come with the !important flag, so they automatically get highest in the CSS priority order, and are hard to understand (what does 1 actually mean in mx-1 - I have to look that up at bootstraps documentation...), but that's a question of personal taste. I think it's fine for now, but we'll have to discuss it for the future.

frontend/src/components/QuickAdd.tsx Outdated Show resolved Hide resolved
@jonandernovella
Copy link
Contributor

Why not use normal paddings and margins? is there a benefit on using the bootstrap classes?

@KattisLej
Copy link
Contributor Author

@jonandernovella Yes, I think it's a benefit in using the Bootstrap classes, I've learned to use them as best practice in Bootstrap. The alternative imo is creating more clutter in the code. But I can change it to several css classes np. :)

@kusalananda
Copy link
Member

Looks nice now. 👍

@ghost ghost dismissed HenrikeW’s stale review May 24, 2022 07:05

requested by HenrikeW

@ghost ghost merged commit 33b8708 into develop May 24, 2022
@ghost ghost deleted the fix/333-proper-label-placeholder-for-quickadd-input branch May 24, 2022 07:05
@jonandernovella jonandernovella mentioned this pull request May 24, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility frontend This is likely affecting the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proper label + placeholder for quickadd input
4 participants