vikunja/frontend
Archived
6
39
Fork 48
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

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

Merged
konrad merged 5 commits from dpschen/frontend:feature/add-hint-modal-variant-to-modal into main 2021-09-24 18:08:49 +00:00
Conversation 7 Commits 5 Files Changed 17 +258 -201
dpschen commented 2021-09-20 15:02:10 +00:00
Member
Copy Link

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
fix: import bulma utilities global
All checks were successful
continuous-integration/drone/pr Build is passing
Details
7955abc28a 
SCSS default values do have to be defined before; see: https://sass-lang.com/documentation/variables#default-values
feat: add variant hint-modal to modal component
All checks were successful
continuous-integration/drone/pr Build is passing
Details
9a67e8f8b3 
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
Owner
Copy Link

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

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
Author
Member
Copy Link

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
Screenshot 2021-09-21 at 21.22.45.png
konrad commented 2021-09-21 19:47:52 +00:00
Owner
Copy Link

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
Author
Member
Copy Link

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
Owner
Copy Link

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
Author
Member
Copy Link

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
Owner

Looks good now. Thanks!

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
This repo is archived. You cannot comment on pull requests.
Reviewers
No Reviewers
konrad
Labels
Clear labels
area/internal-code
changes requested
confirmed
dependencies
duplicate
This issue or pull request already exists
good first issue
help wanted
Need some help
hosting
invalid
Something is wrong
kind/bug
Something is not working
kind/feature
New feature
question
More information is needed
wontfix
This won't be fixed
No Label
changes requested
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dpschen frederick konrad renovate
No Assignees
2 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/frontend#764
No description provided.
Delete Branch "dpschen/frontend:feature/add-hint-modal-variant-to-modal"

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?