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

feat: add message component #1082

Merged
dpschen merged 10 commits from feature/message into main 2021-11-28 14:18:28 +00:00
Conversation 5 Commits 10 Files Changed 23 +170 -127
konrad commented 2021-11-24 21:24:13 +00:00
Owner
Copy Link

This PR adds a simple message component that replaces bulma's default message.

This PR adds a simple message component that replaces bulma's default message.
konrad added 1 commit 2021-11-24 21:24:14 +00:00
continuous-integration/drone/pr Build is passing Details
72b44567b8
feat: add message component
frederick commented 2021-11-24 21:39:51 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1082-featuremessage--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!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1082-featuremessage--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! > Beep boop, I'm a bot.
dpschen requested changes 2021-11-26 11:47:46 +00:00
dpschen left a comment
Member

The message styles are still used in:

  • MigrateService.vue

Use new component also for (and adapt styles):

  • offline-message in ready.vue
  • error / success messages in (not sure here)
    • add-task.vue
    • LinkSharingAuth.vue
    • Login.vue
    • OpenIdAuth.vue
    • PasswordReset.vue
    • Register.vue
    • ... (probably missed some her)

Remove message styles from notification.scss (maybe it can removed completely – not sure what the remaining styles are for).

The notification class also seems super related. I think those should be merged (?).

The message styles are still used in: - MigrateService.vue Use new component also for (and adapt styles): - offline-message in ready.vue - error / success messages in (not sure here) - add-task.vue - LinkSharingAuth.vue - Login.vue - OpenIdAuth.vue - PasswordReset.vue - Register.vue - ... (probably missed some her) Remove message styles from notification.scss (maybe it can removed completely – not sure what the remaining styles are for). The notification class also seems super related. I think those should be merged (?).
src/components/misc/message.vue
@ -0,0 +15,4 @@
padding: .75rem 1rem;
border: 1px solid var(--primary);
background: hsla(var(--primary-hsl), .05);
margin: .5rem 0;
dpschen commented 2021-11-26 11:38:22 +00:00
Member

Remove margin (we don't know all contexts this component will be used in).

Remove margin (we don't know all contexts this component will be used in).
konrad commented 2021-11-26 20:47:54 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
konrad was assigned by dpschen 2021-11-26 11:48:50 +00:00
konrad added 1 commit 2021-11-26 20:47:51 +00:00
continuous-integration/drone/pr Build is passing Details
86bea079f0
chore: move margin
konrad added 1 commit 2021-11-26 21:21:06 +00:00
konrad added 1 commit 2021-11-26 21:23:18 +00:00
continuous-integration/drone/pr Build is failing Details
de090db794
feat: replace usage of bulma's message
konrad added 1 commit 2021-11-26 21:24:02 +00:00
continuous-integration/drone/pr Build is failing Details
dc64e9c3ba
chore: cleanup
konrad commented 2021-11-26 21:27:48 +00:00
Author
Owner
Copy Link

I've moved all uses of bulma's message and notification classes to the new message component while adding a few variants for success, danger and warning messages.

offline-message in ready.vue

I'd like to keep that - it wasn't using the "old" message style before anyway.

Remove message styles from notification.scss (maybe it can removed completely – not sure what the remaining styles are for).

I couldn't find any other uses of it and removed it.

The notification class also seems super related. I think those should be merged (?).

Bulma has a concept of a message and a notification. Both are similar but not the same. I think it makes sense to unify them and have only one notification style message component. The one component I added for that looks a lot cleaner as well IMHO.

I've moved all uses of bulma's message and notification classes to the new message component while adding a few variants for success, danger and warning messages. > offline-message in ready.vue I'd like to keep that - it wasn't using the "old" message style before anyway. > Remove message styles from notification.scss (maybe it can removed completely – not sure what the remaining styles are for). I couldn't find any other uses of it and removed it. > The notification class also seems super related. I think those should be merged (?). Bulma has a concept of a [message](https://bulma.io/documentation/components/message/) and a [notification](https://bulma.io/documentation/elements/notification/). Both are similar but not the same. I think it makes sense to unify them and have only one notification style message component. The one component I added for that looks a lot cleaner as well IMHO.
👍 1
konrad requested review from dpschen 2021-11-26 21:27:53 +00:00
dpschen requested changes 2021-11-26 21:48:50 +00:00
src/components/misc/message.vue Outdated
@ -0,0 +1,41 @@
<template>
<div class="message" :class="[is]">
dpschen commented 2021-11-26 21:40:01 +00:00
Member

picky: remove brachets

picky: remove brachets
konrad commented 2021-11-28 11:19:13 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
src/components/misc/message.vue Outdated
@ -0,0 +6,4 @@
<script setup>
const props = defineProps({
is: {
dpschen commented 2021-11-26 21:42:13 +00:00
Member

is as a prop is already used by vues component component. There it indicates the component / tag. Since the meaning of this prop name is already taken I think we should use something different here. E.g. variant which is used in the modal.vue.

`is` as a prop is already used by vues `component` component. There it indicates the component / tag. Since the meaning of this prop name is already taken I think we should use something different here. E.g. `variant` which is used in the modal.vue.
konrad commented 2021-11-28 11:22:03 +00:00
Author
Owner

I like variant - changed it.

I like `variant` - changed it.
konrad marked this conversation as resolved
src/components/misc/message.vue Outdated
@ -0,0 +18,4 @@
padding: .75rem 1rem;
border-radius: $radius;
&, &.info {
dpschen commented 2021-11-26 21:39:14 +00:00
Member

No need for the first &: the default prop already solves this.

No need for the first `&`: the default prop already solves this.
konrad commented 2021-11-28 11:19:20 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
src/components/misc/message.vue Outdated
@ -0,0 +23,4 @@
background: hsla(var(--primary-hsl), .05);
}
&.danger {
dpschen commented 2021-11-26 21:39:40 +00:00
Member

Don't indent classes: adds more (not needed) specifity.

Don't indent classes: adds more (not needed) specifity.
konrad commented 2021-11-28 11:19:25 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen commented 2021-11-26 22:09:04 +00:00
Member
Copy Link

offline-message in ready.vue

I'd like to keep that - it wasn't using the "old" message style before anyway.

Sure 👍

I couldn't find any other uses of it and removed it.

🥳

Bulma has a concept of a message and a notification. Both are similar but not the same. I think it makes sense to unify them and have only one notification style message component. The one component I added for that looks a lot cleaner as well IMHO.

I didn't even completelty realize this :D
But much better this way =)

> offline-message in ready.vue > I'd like to keep that - it wasn't using the "old" message style before anyway. Sure 👍 > I couldn't find any other uses of it and removed it. 🥳 > Bulma has a concept of a message and a notification. Both are similar but not the same. I think it makes sense to unify them and have only one notification style message component. The one component I added for that looks a lot cleaner as well IMHO. I didn't even completelty realize this :D But much better this way =)
konrad added 1 commit 2021-11-28 11:19:11 +00:00
continuous-integration/drone/pr Build encountered an error Details
44deedd0e5
chore: cleanup class usage
konrad added 1 commit 2021-11-28 11:21:49 +00:00
konrad added 1 commit 2021-11-28 11:27:10 +00:00
continuous-integration/drone/pr Build is failing Details
8143beb44c
chore: fix lint
konrad requested review from dpschen 2021-11-28 11:33:56 +00:00
dpschen requested changes 2021-11-28 12:46:28 +00:00
dpschen left a comment
Member

After that small fix all good to merge =)

After that small fix all good to merge =)
src/components/misc/message.vue Outdated
@ -0,0 +6,4 @@
<script setup>
// eslint-disable-next-line no-unused-vars
const props = defineProps({
dpschen commented 2021-11-28 12:19:42 +00:00
Member

Disabling might be prevented by removing const props = .

Disabling might be prevented by removing `const props = `.
konrad commented 2021-11-28 13:32:08 +00:00
Author
Owner

That seems to work! I thought setting the prop with const props = ... was required for props to work.

That seems to work! I thought setting the prop with `const props = ...` was required for props to work.
👍 1
dpschen commented 2021-11-28 14:17:24 +00:00
Member

The props are automatically exposed via the context

The props [are automatically exposed via the context](https://vue-next-template-explorer.netlify.app/#%7B%22src%22%3A%22%3Ctemplate%3E%5Cn%5Ct%3Cdiv%20class%3D%5C%22message%5C%22%20%3Aclass%3D%5C%22variant%5C%22%3E%5Cn%20%20%20%20%7B%7Bbla%7D%7D%5Cn%5Ct%5Ct%3Cslot%2F%3E%5Cn%5Ct%3C%2Fdiv%3E%5Cn%3C%2Ftemplate%3E%22%2C%22options%22%3A%7B%22mode%22%3A%22module%22%2C%22filename%22%3A%22Foo.vue%22%2C%22prefixIdentifiers%22%3Afalse%2C%22hoistStatic%22%3Afalse%2C%22cacheHandlers%22%3Afalse%2C%22scopeId%22%3Anull%2C%22inline%22%3Afalse%2C%22ssrCssVars%22%3A%22%7B%20color%20%7D%22%2C%22compatConfig%22%3A%7B%22MODE%22%3A3%7D%2C%22whitespace%22%3A%22condense%22%2C%22bindingMetadata%22%3A%7B%22TestComponent%22%3A%22setup-const%22%2C%22setupRef%22%3A%22setup-ref%22%2C%22setupConst%22%3A%22setup-const%22%2C%22setupLet%22%3A%22setup-let%22%2C%22setupMaybeRef%22%3A%22setup-maybe-ref%22%2C%22setupProp%22%3A%22props%22%2C%22vMySetupDir%22%3A%22setup-const%22%7D%7D%7D)
konrad commented 2021-11-28 14:20:24 +00:00
Author
Owner

Neat, I didn't know that.

Neat, I didn't know that.
dpschen commented 2021-11-28 14:22:24 +00:00
Member

I think that's a composition api only thing. AFAIK with the option api you get the props in the render function via that $props argument.

I think that's a composition api only thing. AFAIK with the option api you get the props in the render function via that `$props` argument.
👍 1
konrad marked this conversation as resolved
dpschen commented 2021-11-28 12:56:57 +00:00
Member
Copy Link

Just saw: the tests also seem to need an adjustment

Just saw: the tests also seem to need an adjustment
konrad added 1 commit 2021-11-28 13:31:28 +00:00
continuous-integration/drone/pr Build was killed Details
9b130919d7
fix: eslint workaround
konrad added 1 commit 2021-11-28 13:35:42 +00:00
continuous-integration/drone/pr Build is passing Details
e8a9f69fbb
fix: tests
konrad commented 2021-11-28 13:36:23 +00:00
Author
Owner
Copy Link

Tests should be fixed now.

Tests should be fixed now.
👍 1
konrad requested review from dpschen 2021-11-28 13:36:26 +00:00
dpschen approved these changes 2021-11-28 14:17:53 +00:00
dpschen merged commit f8d009a6aa into main 2021-11-28 14:18:28 +00:00
dpschen deleted branch feature/message 2021-11-28 14:18:28 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
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
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1082
No description provided.
Delete Branch "feature/message"

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?