feat(Wizard): migrate to tailwind #4285
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-tw-wizard.surge.sh |
|
Size Change: -1.63 kB (0%) Total Size: 441 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
107ba47
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ca2a536c.orbit.pages.dev |
| Branch Preview URL: | https://tw-wizard.orbit.pages.dev |
|
Hi @vbulant, I see this PR is in draft, are you working on it? Do you need any help from our side? Just let us know :) |
Hello @mvidalgarcia, it’s mostly done. There’s just a weird discrepancy in visual tests in the pipeline. I’m now working on something else but gonna get back to it tomorrow the latest. Gonna let you know in case I need help 🙂 |
There are regressions in I’ve compared the rendered markup with prod and can’t see any difference though. Not sure if it’s just some text rendering glitch or if I’m missing something. Could you check as well? 🙏 |
|
@vbulant you can see the diff here https://kiwicom-orbit-test-report-tw-wizard.surge.sh/#?q=s:failed it seems to be something with sizing or spacing or something (I didn't check the code, just the snapshots) Ah and you can ignore the Docs job failing, that is on us |
Yeah I know. I already checked the diffs and couldn’t find any difference in spacings or anything else. Gonna try again tomorrow. |
|
Ok, I will also take a look and try to spot the cause for the difference |
DSil
reviewed
Mar 19, 2024
There was a problem hiding this comment.
I have left some comments of small improvements but overall great work.
In the meantime, after a lot of investigation, I have found the problem with the tests. It seems to be a problem with Playwright itself 🤯
This line was not being evaluated correctly on the browser Playwright uses for testing:
So the padding was missing!
When using tailwind, the padding is introduced, causing the text to wrap because it does not fit in one line anymore.
I have verified and in Storybook the padding is being applied correctly with styled components, so it is something we want to keep indeed.
Therefore, you can trust the changed snapshots and update them (both darwin and linux) 👍 🙏
| className={cx( | ||
| "block [&_span]:block", | ||
| !isActive && | ||
| "group-hover/button:[&_span]:text-primary-foreground group-focus/button:[&_span]:text-primary-foreground group-hover/button:[&_span]:underline group-focus/button:[&_span]:underline", |
There was a problem hiding this comment.
It is a bit nitpick, but these hover and focus styles only applied on desktop and above. With this, this is being applied in tablet as well. I don't think there is a major problem, but I don't know how weird that effect might be on a touch device like a tablet.
To avoid any complications, and to keep 100% match, do you think you can add a desktop selector to these classes?
There was a problem hiding this comment.
I don’t think breakpoints smaller than desktop imply touch device. You can still have a small window even on a computer with pointer device. Thus I prefer not restricting hover/focus on specific media queries. But if you prefer 100% match I’ll tweak it.
There was a problem hiding this comment.
My point was more the opposite: if we only have it for desktop and above, it is very unlikely to have hover effects on a touch device. But for the sake of simplification I don't mind keeping your approach as is 👍
There was a problem hiding this comment.
Not sure if I follow but afaik the row timeline with hovers is visible from lm: and above. But anyway let’s keep it simple then 👍
vbulant
force-pushed
the
tw/wizard
branch
3 times, most recently
from
697810e to
90fd41a
Compare
|
@DSil and thank you for the insightful investigation! |
DSil
reviewed
Mar 21, 2024
There was a problem hiding this comment.
I failed to review this on another components, but this needs to be on a different commit.
We have one commit per package (except for docs, that one does not matter), so that the changelog produces only messages based on the commits from those packages. Therefore, please move this (and the other) file to a separate commit, as a chore commit, stating the addition of two new classes.
It is a chore commit instead of a feat because we don't want component-specific classes to be tracked, as we are free to change them without implying breaking changes
There was a problem hiding this comment.
Got it, I’ve moved these changes to a chore commit.
| className={cx( | ||
| "block [&_span]:block", | ||
| !isActive && | ||
| "group-hover/button:[&_span]:text-primary-foreground group-focus/button:[&_span]:text-primary-foreground group-hover/button:[&_span]:underline group-focus/button:[&_span]:underline", |
There was a problem hiding this comment.
My point was more the opposite: if we only have it for desktop and above, it is very unlikely to have hover effects on a touch device. But for the sake of simplification I don't mind keeping your approach as is 👍
DSil
force-pushed
the
tw/wizard
branch
2 times, most recently
from
31d97be to
107ba47
Compare
DSil
approved these changes
Mar 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes FEPLT-1705
This Pull Request meets the following criteria:
For Tailwind migrations:
tailwind-migration-status.yamlfile has been updated with the migrated component(s).For new components:
d.tsfiles and are exported inindex.d.ts