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

Feature/v3 colors and tweaks #31

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

jordythevulder
Copy link
Collaborator

Implemented v3 and various tweaks

@@ -1,5 +1,5 @@
<div class="flex items-center justify-between gap-10 font-medium max-md:flex-1 md:ml-auto">
<div class="h-12 w-16 border flex items-center rounded-md justify-center text-ct-neutral ring ring-ct-inactive-100 text-base">
<div class="h-12 w-16 border flex items-center rounded-md justify-center ring ring-background text-base">
Copy link
Member

Choose a reason for hiding this comment

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

Ok for now, internal reference: RAP-1079

Copy link
Member

Choose a reason for hiding this comment

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

This is within the core now, maybe search and replace all -backgrounds?

@royduin
Copy link
Member

royduin commented Dec 18, 2024

And the readme needs an update.

@royduin
Copy link
Member

royduin commented Jan 14, 2025

Does it depend on a specific version of one of these?

  • rapidez/checkout-theme
  • rapidez/core
  • rapidez/blade-components

The currently only dependency is: "rapidez/checkout-theme": "^3.0", so with just rapidez/checkout-theme version 3.0.0 everything works?

@jordythevulder
Copy link
Collaborator Author

Does it depend on a specific version of one of these?

  • rapidez/checkout-theme
  • rapidez/core
  • rapidez/blade-components

The currently only dependency is: "rapidez/checkout-theme": "^3.0", so with just rapidez/checkout-theme version 3.0.0 everything works?

I've used these versions locally
Checkout theme dev-master as 3.0
Core dev-master as 3.0
Blade components dev-master as 1.2.0

@royduin
Copy link
Member

royduin commented Jan 15, 2025

Let's wait for rapidez/checkout-theme#160 to be merged and tagged as this depends on it.

@jordythevulder
Copy link
Collaborator Author

jordythevulder commented Jan 29, 2025

Found a few classes that weren't necessary, also double checked the PR. Looks good to me :)

@Roene-JustBetter
Copy link
Member

  • Fixed merge conflict.
  • Added sorting for country select.
  • Checked translations and added / removed it.

With the fixes above this PR should be ready for merge 🚀

@jordythevulder
Copy link
Collaborator Author

  • Fixed merge conflict.
  • Added sorting for country select.
  • Checked translations and added / removed it.

With the fixes above this PR should be ready for merge 🚀

Perfect! looks great :)

@@ -9,5 +9,5 @@
@include('rapidez-ct::checkout.partials.address-card')
</x-rapidez-ct::card.inactive>
</x-rapidez-ct::sections>
<label class="absolute cursor-pointer inset-0 bg-ct-neutral/60" for="popup"></label>
<label class="absolute cursor-pointer inset-0 bg-black/60" for="popup"></label>
Copy link
Member

Choose a reason for hiding this comment

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

Is this a backdrop? In that case we do have a backdrop color for that.

Comment on lines +29 to +30
</dl>
<dl v-if="order.grand_total" class="flex items-center order-last justify-between border-t border-dashed pt-3 mt-4">
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 2 dl's? Can't we have everything in one dl?

Comment on lines +44 to +46
/>
</div>
</x-rapidez-ct::input.radio.tile>
Copy link
Member

Choose a reason for hiding this comment

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

Is this indenting right?

@@ -21,7 +34,7 @@
</div>
</div>
</div>
<button v-if="cart.items.length > 2" class="mt-2 flex items-center pb-1 gap-1 text-ct-inactive font-medium" @click="toggle">
<button v-if="cart.items.length > 2" class="flex items-center pb-1 gap-1 !border-t-0 !pt-0 hover:underline" @click="toggle">
Copy link
Member

Choose a reason for hiding this comment

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

Importants on a default button?

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is because of the wrapping <div class="*:pt-2.5 first:*:pt-0 mb-2.5 flex w-full flex-col gap-2.5 divide-y divide-dashed border-b border-dashed pb-2.5"> that sets borders between the elements.

The button here as you can see below doesn't need a border as that looks weird:
Scherm­afbeelding 2025-02-26 om 10 29 37
Scherm­afbeelding 2025-02-26 om 10 31 30

Scherm­afbeelding 2025-02-26 om 10 29 56 Scherm­afbeelding 2025-02-26 om 10 31 22

@@ -7,7 +7,7 @@
</x-rapidez-ct::title>

<x-rapidez-ct::sections>
<x-rapidez-ct::card.inactive class="!bg-ct-primary-100 py-8 px-6 rounded-xl border-b border-black/5">
<x-rapidez-ct::card.inactive class="!bg-green-400/15 px-7 !py-5 rounded-xl border-b border-black/5">
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we using the Tailwind merging so we don't need any importants?

Copy link
Member

Choose a reason for hiding this comment

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

I checked the importants. For the background we don't need the !important so we can remove it. For the padding we need the important because of the wrapping parent divide-y divide-dashed *:pt-6 *:mb-6 mt-5 mb-6 first:*:pt-0.

@@ -1,9 +1,9 @@
@props(['check' => false])
<div {{ $attributes->class('relative flex-1 rounded-md border bg-white px-5 py-6 overflow-hidden ring ring-ct-inactive-100') }}>
<div {{ $attributes->class('relative flex-1 rounded-md border bg-white px-6 py-5 overflow-hidden ring ring-background') }}>
Copy link
Member

Choose a reason for hiding this comment

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

ring-background? Shouldn't that be ring-muted?

@@ -1,6 +1,6 @@
@props(['customTitle' => null])

<x-rapidez-ct::card.white class="!border-0 !shadow-none !p-0" {{ $attributes->only('v-if') }}>
<x-rapidez-ct::card.white class="!border-0 !shadow-none !p-0 !ring-0" {{ $attributes->only('v-if') }}>
Copy link
Member

Choose a reason for hiding this comment

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

We need to add twMerge so we don't need the importants

@@ -0,0 +1,7 @@
<select {{ $attributes
->except(['v-if', 'v-else', 'v-else-if', 'class'])
->class('w-full cursor-pointer border border-default focus:border-emphasis bg-white pt-7 pb-2.5 px-5 text-sm font-medium rounded-m bg-white outline-none transition-all disabled:bg-emphasis !ring !ring-background focus:!ring-background-emphasis rounded-md') }}>
Copy link
Member

Choose a reason for hiding this comment

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

ring-background-emphasis => ring / ring-default?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants