activist-org / activist

An open-source activism platform
https://activist.org
GNU Affero General Public License v3.0
222 stars 184 forks source link

add frontend formatting check #323

Closed to-sta closed 1 year ago

to-sta commented 1 year ago

Terms

Description

In the styleguide prettier and headwind is mention for formatting frontend code. Would make sense to implement a workflow to ensure its conform with the styleguide hence prettier and headwind.

Contribution

I can help with creating a GitHub workflow. I am sure i am able to setup workflow with prettier but i have to do some research on how to incorporate headwind as well.

andrewtavis commented 1 year ago

Thanks for the issue, @to-sta! Just assigned it to you :) As far as headwind is concerned, don’t worry about the check too much. Headwind’s mostly good, but can be a bit random when you mix in custom classes that we use extensively. It’s been very normal that I do a headwind format and a file that no one has touched for a while gets formatted πŸ€·β€β™‚οΈ

It is really helpful, but I can just do some headwind formatting commits from time to time 😊 Prettier would be great though!

andrewtavis commented 1 year ago

f80d9a5 sent through fixes just now, @to-sta!

andrewtavis commented 1 year ago

Note that we have some errors being reported in the most recent PR (#326), @to-sta: https://github.com/activist-org/activist/actions/runs/6041410258/job/16394314324

Is this something we should try to fix on our end, or would you say we should try to ignore something on the Prettier side?

to-sta commented 1 year ago

It worked πŸ˜ƒ.

Here are warnings:

[warn] frontend/components/grid/GridGithubShields.vue
[warn] frontend/components/Loading.vue
[warn] frontend/components/media/MediaMap.vue
[warn] frontend/components/PageBreadcrumbs.vue
[warn] frontend/components/sidebar/right/SidebarRight.vue
[warn] frontend/composables/useFormCheckboxRadio.ts
[warn] frontend/composables/useFormSetup.ts
[warn] frontend/pages/home/index.vue
[warn] frontend/tsconfig.json

These should be all solved by formatting the files with prettier. I tried the first one and it only changes the formatting.

Here are the errors:

Error:  frontend/types/event.d.ts: SyntaxError: A property signature cannot have an initializer. (5:18)
Error:    3 |   tagline: string;
Error:    4 |   organizer: string;
Error:  > 5 |   type: string = "act" | "learn";
Error:      |                  ^^^^^^^^^^^^^^^
Error:    6 |   topic: string;
Error:    7 |   description: string;
Error:    8 |   getInvolvedDescription: string;
Error:  frontend/types/feed.d.ts: SyntaxError: A property signature cannot have an initializer. (2:22)
Error:    1 | export interface Feed {
Error:  > 2 |   itemType: string = "groups" | "social posts";
Error:      |                      ^^^^^^^^^^^^^^^^^^^^^^^^^
Error:    3 | }
Error:    4 |
Error:  frontend/types/feedItem.d.ts: SyntaxError: A property signature cannot have an initializer. (3:22)
Error:    1 | export interface FeedItem {
Error:    2 |   url: string;
Error:  > 3 |   itemType: string = "group" | "social post";
Error:      |                      ^^^^^^^^^^^^^^^^^^^^^^^
Error:    4 |   title: string;
Error:    5 |   description: string;
Error:    6 |   imgURL?: string;

I am not that familiar with typescript yet, would be good if someone else can jump in here.

From a quick google search i found:

You cannot provide default values for interfaces or type aliases as they are compile time only and default values need runtime support

andrewtavis commented 1 year ago

These should be all solved by formatting the files with prettier. I tried the first one and it only changes the formatting.

Weird thing is that when I try to use Prettier on these files nothing happens πŸ€”

You cannot provide default values for interfaces or type aliases as they are compile time only and default values need runtime support

I'll do a fix for this!

andrewtavis commented 1 year ago

e899cd7 removed those optional assignments from the .d.ts files, @to-sta. Am still not sure what to do about the Prettier warnings as those files seem to not need to be formatted πŸ€”

to-sta commented 1 year ago

Weird thing is that when I try to use Prettier on these files nothing happens πŸ€”

You are right @andrewtavis. I have only tested the first [warn] file. For the rest of the files nothing happend on my side either. If i install prettier via npm and run it, then i also get [warn] and [error] logs. So there is a difference between running the prettier VScode extension without installing it via npm (the VScode extension will look for the prettier module in the node_modules file).

I will have look that again in the next couple of days. For now i would say we ignore the frontend checks.

to-sta commented 1 year ago

So the current issue is alignment:

  1. VSCode Extension and GitHub Workflow
  2. Different VSCode Extension settings
  3. Different IDE's
  4. Different versions of prettier

I think to streamline this process we should install prettier via npm. That way we achieve a single source of truth inform of a .prettierignore and .prettierrc (config file). Then we adjust the Github workflow to also use the repo's configuration.

The VSCode extension will by default look for node_modules, package.json, .prettierignore and .prettierrc in the root of the repo. We can add a settings.json file to the .vscode folder with the following settings:

{
    "prettier.configPath": "frontend/.prettierrc",
    "prettier.ignorePath": "frontend/.prettierignore"
}

or move the files to the root to reduce friction for contributors.

Let me know what you think @andrewtavis?

andrewtavis commented 1 year ago

This all looks really good, @to-sta! Makes sense that it’s all weird because of versions and such :) I was going to suggest looking into a hard coded configuration file for prettier 😊 Do you want to continue with this? Anything I can do to help?

to-sta commented 1 year ago

I will make a PR with the changes mentioned above.

andrewtavis commented 1 year ago

Sounds good, @to-sta! Looking forward to it 😊

andrewtavis commented 1 year ago

I think that this one is done, right @to-sta? If so, do you want to close it? πŸ˜‰ You can, right?

andrewtavis commented 1 year ago

πŸ”₯😊