Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate from Windi to Tailwind #4614

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Migrate from Windi to Tailwind #4614

wants to merge 38 commits into from

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Dec 23, 2024

superseedes #3279

fixes #2194

Had to add some scoped classes to resolve some padding and override clashes.
In addition, some width and height specifiers had to be changed as they are different between windi and tailwind.

@pat-s pat-s added refactor delete or replace old code build_pr_images If set, the CI will build images for this PR and push to Dockerhub labels Dec 23, 2024
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Dec 23, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-4614.surge.sh

@pat-s pat-s force-pushed the refactor/windi-to-tailwind branch from 15912d6 to 6e64b94 Compare December 23, 2024 17:30
@pat-s pat-s marked this pull request as ready for review December 23, 2024 23:21
.yamllint.yaml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
web/src/views/admin/AdminRepos.vue Show resolved Hide resolved
web/vite.config.ts Show resolved Hide resolved
@pat-s pat-s mentioned this pull request Dec 24, 2024
4 tasks
xoxys

This comment was marked as resolved.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 28, 2024

@xoxys @anbraten @qwerty287 Could you please check here again? Rebasing against renovate updates is a bit tedious, hence it would be great if we can get this merged before the next round of npm updates.

.yamllint.yaml Outdated Show resolved Hide resolved
web/src/style.css Outdated Show resolved Hide resolved
web/tailwind.config.js Outdated Show resolved Hide resolved
@pat-s
Copy link
Contributor Author

pat-s commented Dec 28, 2024

@xoxys If you wanna continue and resolve the new issue after merging main, feel free to. I am away until the end of the day.

@xoxys xoxys added the ui frontend related label Dec 28, 2024
@@ -3,6 +3,7 @@
<PipelineItem
v-for="pipeline in pipelines"
:key="pipeline.id"
class="pipeline-list"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class doesn't exist? Where is it coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be an oversight.

@@ -65,3 +65,9 @@ defineEmits<{
const date = useDate();
</script>

<style scoped>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this here? Applying p-4 directly just works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the devtools it works, it didn't when applying it in the code and then building. Feel free to try again, I tried multiple times but maybe I made a mistake or overlooked something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I alway build it locally not using dev tools.

Copy link
Contributor Author

@pat-s pat-s Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter how you build it, inspecting a deployed instance vs localhost is the same in the end. Maybe we're talking about different things?
I don't think this matter though. Again, if you find a way to include it as a class directly, this would be great and better than this, but it didn't work for me.

Comment on lines +2 to +3
<main class="flex flex-col justify-center items-center w-full h-full">
<Error v-if="errorMessage" class="w-full md:max-w-3xl h-96">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change?

>
<span>{{ errorUri }}</span>
</a>
</Error>

<div
class="flex flex-col w-full overflow-hidden bg-wp-background-100 shadow border border-wp-background-400 dark:bg-wp-background-200 md:m-8 md:rounded-md md:flex-row md:w-3xl md:h-sm"
class="flex md:flex-row flex-col border-wp-background-400 bg-wp-background-100 dark:bg-wp-background-200 shadow md:m-8 border md:rounded-md w-full md:max-w-3xl md:h-96 overflow-hidden"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Why do we need to change it to max-width?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the Login item is fully horizontally stretched across the page with Tailwind.

@@ -66,3 +66,9 @@ const { doSubmit: deleteOrg, isLoading: isDeleting } = useAsyncAction(async (_or
resetPage();
});
</script>

<style scoped>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here, why do we need a scoped class for a single property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't work with overrides or normal injection. Classes collided with ones merged from parent divs and didn't take effect. Scoped classes seem to solve this.
If you know a better way to solve this, please propose it.

@@ -154,3 +154,9 @@ function showAddUser() {
selectedUser.value = cloneDeep({ login: '' });
}
</script>

<style scoped>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Comment on lines +209 to +211
borderWidth: {
6: '6px',
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be new. Where is it coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a custom class required to make some existing custom windi classes work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add more details? Where do you got this from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no tailwind class with a border width of 6px.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I still dont get it. Where exactly do we need a border with the width of 6px and how is it used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class="flex items-center gap-2 border-wp-error-300 dark:border-wp-error-300 bg-wp-error-100 dark:bg-wp-error-200 p-2 border border-l-6 border-solid rounded-md text-white"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then is suggest to use 4 or 8 if thats the only location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another one, best to grep for it. Maybe something for a different PR?

web/vite.config.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_pr_images If set, the CI will build images for this PR and push to Dockerhub refactor delete or replace old code ui frontend related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to TailwindCSS
4 participants