feat: create BaseButton component #1123

dpschen · 2021-11-30T20:38:33Z

dpschen commented

2021-11-30 20:38:33 +00:00

No description provided.

dpschen added the

kind/feature

label 2021-11-30 20:38:33 +00:00

dpschen self-assigned this 2021-11-30 20:38:33 +00:00

frederick commented

2021-11-30 20:52:50 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1123-featurefeat-BaseButton-component--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 dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1123-featurefeat-BaseButton-component--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 force-pushed feature/feat-BaseButton-component from 47441a55bb to c526d90256

2021-12-22 15:20:42 +00:00

Compare

dpschen changed title from ~~WIP: feat: create BaseButton component~~ to feat: create BaseButton component

2021-12-22 15:50:57 +00:00

dpschen requested review from konrad 2021-12-22 15:51:03 +00:00

konrad was assigned by dpschen

2021-12-22 15:51:06 +00:00

dpschen force-pushed feature/feat-BaseButton-component from 1327172494 to d2cdb30519

2021-12-22 15:59:53 +00:00

Compare

dpschen force-pushed feature/feat-BaseButton-component from d2cdb30519 to 40c75e92fd

2021-12-22 16:07:48 +00:00

Compare

dpschen force-pushed feature/feat-BaseButton-component from a47cd0d0c9 to e242ef38a3

2021-12-22 19:00:00 +00:00

Compare

konrad reviewed 2021-12-23 08:59:43 +00:00

src/components/base/BaseButton.vue Outdated

						
				@ -0,0 +75,4 @@

				// NOTE: we do not use scoped styles to reduce specifity and make it easy to overwrite

				// We reset the default styles of a button element to enable easier styling

				:where(.base-button--type-button) {

konrad commented

2021-12-23 08:56:49 +00:00

Why the :where() instead of just using .base-button--type-button directly?

Why the `:where()` instead of just using `.base-button--type-button` directly?

dpschen commented

2021-12-23 09:56:55 +00:00

By using :where as the main element class we make it possible to easily pverwrite the seit properties by a parent element. The reason beinh that the specifity of a class wrapped in :where is lower than a class selector in the parent. This way we have sane default styles that are still easy to overwrite (aka without stuff like id selectors, duplicate classes, element selectors or even !important.

By using `:where` as the main element class we make it possible to easily pverwrite the seit properties by a parent element. The reason beinh that the specifity of a class wrapped in `:where` is lower than a class selector in the parent. This way we have sane default styles that are still easy to overwrite (aka without stuff like id selectors, duplicate classes, element selectors or even `!important`.

dpschen marked this conversation as resolved

src/components/misc/create-edit.vue

						
				@ -15,3 +15,4 @@

							<footer class="modal-card-foot is-flex is-justify-content-flex-end">

								<x-button

									v-if="tertiary !== ''"

									:shadow="false"

konrad commented

2021-12-23 08:57:48 +00:00

Isn't this already implicit because of the variant tertary?

Isn't this already implicit because of the variant `tertary`?

dpschen commented

2021-12-23 10:09:35 +00:00

Seems like it. Will remove :)

dpschen commented

2021-12-23 10:11:01 +00:00

Btw it's tertIary. => fixed the spelling

Btw it's tert**I**ary. => fixed the spelling

konrad commented

2021-12-25 20:33:25 +00:00

Btw it's tertIary. => fixed the spelling

Thanks :) I think I got that wrong in a few places...

> Btw it's tertIary. => fixed the spelling Thanks :) I think I got that wrong in a few places...

dpschen marked this conversation as resolved

src/main.ts Outdated

						
				@ -6,3 +4,1 @@

					RENDER_FUNCTION: false,

					WATCH_ARRAY: false, // TODO: check this again; this might lead to some problemes

					TRANSITION_GROUP_ROOT: false,

					MODE: 3,

konrad commented

2021-12-23 08:58:52 +00:00

Is it still required to configure this here even though it is already set in vite's config?

dpschen commented

2021-12-23 10:17:41 +00:00

Would make sense.

I followed this guide (see step 3 under vite).

And then in the same guide under Global Confuguration they tell you to use this ConfuigureCompat method. This is why I made that error.

I guess the property in the vite config is alternative.
The location in vite.config seems better fitted since it's build related, so will remove here :)

Would make sense. I followed [this guide](https://v3.vuejs.org/guide/migration/migration-build.html#preparations) (see step 3 under vite). And then in the same guide under [Global Confuguration](https://v3.vuejs.org/guide/migration/migration-build.html#installation) they tell you to use this ConfuigureCompat method. This is why I made that error. I guess the property in the vite config is alternative. The location in vite.config seems better fitted since it's build related, so will remove here :)

👍 1

dpschen marked this conversation as resolved

konrad commented

2021-12-23 09:00:19 +00:00

So this essentially fixes the a/button distinction?

So this essentially fixes the `a`/`button` distinction?

dpschen commented

2021-12-23 10:07:00 +00:00

So this essentially fixes the a/button distinction?

Yes! It abstracts functionality from style :)

I saw that there are many places in vikunja where link elements are used without a href attribute (which is invalid) and instead an @click listener e.g. to push a route. When pushing routes we should use router-link or maybe later some abstraction if that because it provides optimizations (e.g. the nuxt-link component dynamically preloads linked routes when the element gets in the viewport).

Then there are places where spans / divs are used for click actions. This is not only semantically weong but also has some side effects that are unwanted (e.g. I thinl you have to add the prevent modifier which is not necessary with a native button, that's for free in the platform ™️ ). Where I realized that we need this functionality was when checking the password type toggle buttons: when you click them fast you will select the text in the input area behind. I'm not even sure how that could be prevented in js (it's not user-select: none;

> So this essentially fixes the `a`/`button` distinction? Yes! It abstracts functionality from style :) I saw that there are many places in vikunja where link elements are used without a href attribute (which is invalid) and instead an `@click` listener e.g. to push a route. When pushing routes we should use router-link or maybe later some abstraction if that because it provides optimizations (e.g. the nuxt-link component dynamically preloads linked routes when the element gets in the viewport). Then there are places where spans / divs are used for click actions. This is not only semantically weong but also has some side effects that are unwanted (e.g. I thinl you have to add the prevent modifier which is not necessary with a native button, that's for free in the platform ™️ ). Where I realized that we need this functionality was when checking the password type toggle buttons: when you click them fast you will select the text in the input area behind. I'm not even sure how that could be prevented in js (it's not `user-select: none;`

👍 1

dpschen commented

2021-12-23 10:08:07 +00:00

I think I'll add such a description in the element to make it clearer :)

👍 1

konrad commented

2021-12-25 20:32:41 +00:00

Then there are places where spans / divs are used for click actions. This is not only semantically weong but also has some side effects that are unwanted (e.g. I thinl you have to add the prevent modifier which is not necessary with a native button, that's for free in the platform ™️ ).

Yes, that definitely is something that we should change.

> Then there are places where spans / divs are used for click actions. This is not only semantically weong but also has some side effects that are unwanted (e.g. I thinl you have to add the prevent modifier which is not necessary with a native button, that's for free in the platform ™️ ). Yes, that definitely is something that we should change.

dpschen force-pushed feature/feat-BaseButton-component from f54ac7d062 to f6351e08ab

2021-12-30 14:09:08 +00:00

Compare

dpschen commented

2021-12-30 14:10:39 +00:00

@konrad:

added description and
removed the duplicate compatibility mode definition in main.js

Can you check this again?

@konrad: - added description and - removed the duplicate compatibility mode definition in main.js Can you check this again?

konrad reviewed 2021-12-30 15:22:33 +00:00

src/components/base/BaseButton.vue Outdated

						
				@ -0,0 +34,4 @@

				  submit: 'submit',

				})

				type BaseButtonTypes = keyof typeof BASE_BUTTON_TYPES_MAP

konrad commented

2021-12-30 15:17:42 +00:00

Why not a typescript string enum?

dpschen commented

2021-12-31 00:28:22 +00:00

For this case: might make sense!

Why I still think we should keep it here:
In other components we might have a similar pattern where the values of the map are not strings but objects. E.g. think of a component that changes a configuration based on a prop where you just name the id of the configuration.

const CONFIGURATION_TYPES_MAP = Object.freeze({
	simple: { onlyOneProperty: 1 },
	complex: { here: 1, are: 2, many: 3, properties: 4 }
})

type ConfigurationTypes = keyof typeof CONFIGURATION_TYPES_MAP

AFAIK that would be not possible with enums

For this case: might make sense! Why I still think we should keep it here: In other components we might have a similar pattern where the values of the map are not strings but objects. E.g. think of a component that changes a configuration based on a prop where you just name the id of the configuration. ```ts const CONFIGURATION_TYPES_MAP = Object.freeze({ simple: { onlyOneProperty: 1 }, complex: { here: 1, are: 2, many: 3, properties: 4 } }) type ConfigurationTypes = keyof typeof CONFIGURATION_TYPES_MAP ``` AFAIK that would be not possible with enums

konrad commented

2021-12-31 15:32:58 +00:00

Couldn't you use interfaces with typescript enums for that use case?

dpschen commented

2021-12-31 15:44:34 +00:00

Not sure how you mean that?

An interface would just define the steucture of the configuration object, right?

So by doing this we would not recieve a configuration yet. What I want is the possibilty to chose between predefined configurations via a prop that provides an id (that selects one configuration).

Not sure how you mean that? An interface would just define the steucture of the configuration object, right? So by doing this we would not recieve a configuration yet. What I want is the possibilty to chose between predefined configurations via a prop that provides an id (that selects one configuration).

konrad commented

2021-12-31 15:53:11 +00:00

I had something like this in mind:

interface testIn {
	foo: string
}

enum test {
	one = testIn,
}

That does not work though, so looks like I was wrong. I took a look in the docs and it seems like you can only use numeric or string literals in enums.

Anyway, as you correctly pointed out that would not make sense to do what I had in mind. I think your use case is completely valid here :)

For consistency between the button and base button I think it should be fine to keep this here.

I had something like this in mind: ```ts interface testIn { foo: string } enum test { one = testIn, } ``` That does not work though, so looks like I was wrong. I took a look in the docs and it seems like you can only use numeric or string literals in enums. Anyway, as you correctly pointed out that would not make sense to do what I had in mind. I think your use case is completely valid here :) For consistency between the button and base button I think it should be fine to keep this here.

👍 1

konrad marked this conversation as resolved

dpschen force-pushed feature/feat-BaseButton-component from f6351e08ab to ca51e5b348

2021-12-31 00:32:44 +00:00

Compare

konrad approved these changes 2021-12-31 15:53:27 +00:00

dpschen force-pushed feature/feat-BaseButton-component from ca51e5b348 to a0d91a93b1

2022-01-01 12:51:00 +00:00

Compare

dpschen force-pushed feature/feat-BaseButton-component from a0d91a93b1 to ccd1857867

2022-01-04 18:47:08 +00:00

Compare