feature/add-hint-modal-variant-to-modal #764

dpschen · 2021-09-20T15:02:10Z

dpschen commented

2021-09-20 15:02:10 +00:00

This depends on #738 being merged first

The content inside the modals wasn't indented correctly by intend.

I wanted to make the diff with the currently many other branches simpler. Formatting by indenting is something that we can easily do later.

After finishing I realized that a common pattern when using modals is also that they use a <card> component directly inside.

To reduce duplication it might make sense to create a variant of the modal where the card is included by default.

This depends on https://kolaente.dev/vikunja/frontend/pulls/738 being merged first ---- The content inside the modals wasn't indented correctly by intend. I wanted to make the diff with the currently many other branches simpler. Formatting by indenting is something that we can easily do later. --- After finishing I realized that a common pattern when using modals is also that they use a `<card>` component directly inside. To reduce duplication it might make sense to create a variant of the modal where the card is included by default.

dpschen added 3 commits 2021-09-20 15:02:11 +00:00

continuous-integration/drone/pr Build is passing Details

7955abc28a

fix: import bulma utilities global

SCSS default values do have to be defined before; see: https://sass-lang.com/documentation/variables#default-values

f8044a5caa

feat: import sass:math utilities globally

continuous-integration/drone/pr Build is passing Details

9a67e8f8b3

feat: add variant hint-modal to modal component

NOTE: the content inside the modals wasn't indented correctly by intend. I wanted to make the diff with the currently many other branches simpler. Formatting by indenting is something that we can easily do later.

konrad commented

2021-09-21 17:11:17 +00:00

Looks like this broke the task popup on the kanban board:

Can you look into that?

Looks like this broke the task popup on the kanban board: ![image](/attachments/3e14b074-86c1-4e24-a530-62fdc234c4a3) Can you look into that?

image.png

38 KiB

konrad added the

changes requested

label 2021-09-21 19:09:01 +00:00

dpschen commented

2021-09-21 19:24:38 +00:00

I can't reproduce. How did you get there?

This is after I just checked out the branch again (including the new changes from main).

I can't reproduce. How did you get there? This is after I just checked out the branch again (including the new changes from main).

Screenshot 2021-09-21 at 21.22.45.png

165 KiB

konrad commented

2021-09-21 19:47:52 +00:00

I just checked out the branch, built it and opened a task from the kanban board. Tested on Firefox and Chromium.
See the attached dist build (just throw the contents of the archive into /dist and run yarn serve:dist to host it).

I just checked out the branch, built it and opened a task from the kanban board. Tested on Firefox and Chromium. See the attached dist build (just throw the contents of the archive into `/dist` and run `yarn serve:dist` to host it).

dist.tar.gz

1.5 MiB

dpschen commented

2021-09-22 12:14:43 +00:00

Okay, I could reproduce this now. I just had some local caching issues that were solved after deleting the .vite folder. Kind of obvious what is wrong:

I scoped the modal styles inside of the <modal> component, but they were also used by the <TaskDetailViewModal>.

Now I want to use the <modal> inside of <TaskDetailView> aswell. But I don't understand what the difference is between the <modal> having the overflow prop set to true and the .scrolling-content class of the <TaskDetailViewModal>. Don't they both serve kind of the same purpose?

Or put it in a different way: where would you want your content to just overflow and not scroll when it reaches e.g. the screen border instead of scrolling as it is currently done in the modal?
Sounds like this might be from time to time a potential issue on mobile devices to me.

Okay, I could reproduce this now. I just had some local caching issues that were solved after deleting the `.vite` folder. Kind of obvious what is wrong: I scoped the modal styles inside of the `<modal>` component, but they were also used by the `<TaskDetailViewModal>`. Now I want to use the `<modal>` inside of `<TaskDetailView>` aswell. But I don't understand what the difference is between the `<modal>` having the `overflow` prop set to `true` and the `.scrolling-content` class of the `<TaskDetailViewModal>`. Don't they both serve kind of the same purpose? Or put it in a different way: where would you want your content to just overflow and not scroll when it reaches e.g. the screen border instead of scrolling as it is currently done in the modal? Sounds like this might be from time to time a potential issue on mobile devices to me.

konrad commented

2021-09-22 16:19:13 +00:00

It is very well possible this was an old thing I implemented before the modal component even had the overflow prop. Have you tried if they do different things?

I think you should be able to scroll the task modal away, as opposed to scrolling inside the modal. I think that's what the overflow property does, scrolling inside the modal.

It is very well possible this was an old thing I implemented before the modal component even had the overflow prop. Have you tried if they do different things? I think you should be able to scroll the task modal away, as opposed to scrolling inside the modal. I *think* that's what the overflow property does, scrolling inside the modal.

dpschen force-pushed feature/add-hint-modal-variant-to-modal from ee61d00c9f to 8a0ca3bd1f

2021-09-22 17:53:06 +00:00

Compare

dpschen commented

2021-09-22 17:55:55 +00:00

I fixed it for now by adding another variant to the modal. For the scrolling variant I had to overwrite some bulma styles. This was not necessary before since the .scrolling-container class that was used instead of the .modal-content inside the <TaskDetailViewModal> didn't collide with bulma.

Maybe you can test again :)

For the future it might make sense to remove the amount of variants again.

I fixed it for now by adding another variant to the modal. For the scrolling variant I had to overwrite some bulma styles. This was not necessary before since the `.scrolling-container` class that was used instead of the `.modal-content` inside the `<TaskDetailViewModal>` didn't collide with bulma. Maybe you can test again :) For the future it might make sense to remove the amount of variants again.

konrad approved these changes 2021-09-24 18:07:59 +00:00

konrad left a comment

Looks good now. Thanks!

konrad merged commit 4f2378ff02 into main

2021-09-24 18:08:49 +00:00

konrad deleted branch feature/add-hint-modal-variant-to-modal

2021-09-24 18:08:49 +00:00

konrad referenced this issue from a commit

2021-09-24 18:08:50 +00:00

feat: add variant hint-modal to modal component (#764)

This repo is archived. You cannot comment on pull requests.

No reviewers