feat: use BaseButton where easily possible #1803
No reviewers
konrad
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1803
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/feat-use-BaseButton-where-possible"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This replaces links with BaseButton components. BaseButton will use
<button type="button">inside for this case. This improves accessibility a lot. Also we might be able to remove the.stopmodifiers in some places because AFAIK the button element stops propagation by default.I searched for
@clicklisteners in order to find these cases.It might be that I missed some.
Also there are multiple occasions where an element with a click listener wraps other elements.
This is something that should be avoided whenever possible and instead be implemented by putting an element — our BaseButton — with that click listener inside that spawns over the full parent element size. These kind of fixes are still left, but they should be done since they can be a major source of all kinds of problems (e.g. hitting multiple listeners without knowing at the same time).
Note:
I didn't check for possible style implications yet, though they shouldn't be too big.
Hi dpschen!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1803-feature-feat-use-basebutton-wher--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
7fdbb1ab4cto2eeb843af1WIP: feat: use BaseButton where easily possibleto feat: use BaseButton where easily possibleThe build seems to fail. Please check where the button was already imported.
feat: use BaseButton where easily possibleto WIP: feat: use BaseButton where easily possibleaa5dc7ac96to51dc1edc8a51dc1edc8ato981f059d11Fixed failing tests.
Missing:
WIP: feat: use BaseButton where easily possibleto feat: use BaseButton where easily possible14f8c6e38eto64425c3afa64425c3afato3dec49ee85The styling on empty list pages seems broken:
As well as the edit button:
I think both of these issues come from the fact that a button is a
blockelement and theais aninlineelement. And the default styling for links is (obviously) not applied to a<button>.I missed these, thanks.
Now that I changed everything else:
Do you thing it makes even sense to have the default BaseButton
displayset toblock? Maybeinline-blockwould be better.What breaks when we set it to
inline-block? I think there's places where it is expected to beblock?Yes. I would need to revert some styles, but I think mostly it shouldn't be too bad.
If used inside of flexbox or grid it doesn't matter if display is block or inline-block.
Then I'd say let's try that.
3dec49ee85toc1ff70eea1c1ff70eea1to3f160ba3683f160ba368tocd9b0925af@dpschen Can you resolve the conflicts?
cd9b0925afto8d105df3f8Almost! The styling of the buttons should be the link blue in the cases in the screenshot.
8d105df3f8toc60dee58a95da60935c7tof125ec2045Okay I created a new component called ButtonLink. That's a small wrapper around BaseButton that adds back the link style colors.
Although I tried to check everything I'm not 100% sure if I got all the components where ButtonLink is needed.
Looks like it is still missing here: (the "What?")
Rest looks good!
a2df294385to0a0c046aa1Should be fixed in
0a0c046aa1Looks good! Please fix the CI error, then this should be good to merge.
0a0c046aa1tod8588b9ca0