-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(starter-template): add starter template in TypeScript #2615
Conversation
🦋 Changeset detectedLatest commit: 8a61737 The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit 8a61737. |
0a12339
to
b83357b
Compare
b83357b
to
853bd9f
Compare
5929cb4
to
6374c1e
Compare
6374c1e
to
67138d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super awesome job 💪
I just left some questions/suggestions for better understanding.
...ication-templates/starter-typescript/src/components/channel-details/channel-details-form.tsx
Show resolved
Hide resolved
...ication-templates/starter-typescript/src/components/channel-details/channel-details.spec.tsx
Outdated
Show resolved
Hide resolved
application-templates/starter-typescript/src/test-utils/index.tsx
Outdated
Show resolved
Hide resolved
application-templates/starter-typescript/src/test-utils/test-data/README.md
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,12 @@ | |||
import { Builder } from '@commercetools-test-data/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if we could use the Channel
our test-data
package provides instead of creating this whole directory.
(maybe as a follow-up task)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually tried that and with the current implementation of the Channel
model in @commercetools-test-data/channel
I was not able to get the gql response modelled properly. I haven’t worked with test-data
repo much (perhaps I did not use it properly) but it seems that the Transformer
in its current shape does not allow to customize the name like:
import * as Channel from "@commercetools-test-data/channel";
const channel = Channel.random()
.name(LocalizedString.presets.empty().en(name))
.key("some-key")
.buildGraphql();
The name is always randomly generated so I wouldn’t be able to test the channel name update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show what the error is?
And yes, we should use the model from the test-data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, there’s no error. It only works differently. I created a playground to isolate the issue.
TL;DR: channels transformer from @commercetools-test-data/channel
works differently from the current solution - nameAllLocales
is random, not based on the provided name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems something needs to be adjusted in the test-models
package.
We even have some preset channels over there which doesn't seem to return the expected data:
import * as Channel from "@commercetools-test-data/channel";
const channel: TChannel = Channel.presets.foodStore().buildGraphql();
console.log(
"channel test data:",
{
key: channel.key,
name: JSON.stringify(channel.name),
nameAllLocales: JSON.stringify(channel.nameAllLocales)
}
);
// This is what it renders: (expected other localized names)
channel test data:
{
key: "food-store-key",
name: "{}",
nameAllLocales: "[
{"locale":"de","value":"distinctio nihil magni ut deleniti","__typename":"LocalizedString"},
{"locale":"en","value":"sint aut est","__typename":"LocalizedString"},
{"locale":"fr","value":"eos nihil ratione est","__typename":"LocalizedString"}
]"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested, I used Channel
data model from @commercetools-test-data/channel
in tests (updated in 05150c1).
As noted 👆to make all test cases work (also including parts testing name
and nameAllLocales
that I removed for the time being) that repo would have to be refactored and it seems to me some breaking changes would need to be introduced there (and I guess to other packages in the test data repo too). I don’t feel 100% confident about those changes 😕 as I don't understand why the implementation of the model transformers is different in the first place.
I was wondering if that could be done as a follow up once @emmenko is back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree about working on the test-data repo in a different task as a follow up. However, I would personally keep the tests with our local test-data utilities until we can use the external ones.
application-templates/starter/src/components/channel-details/channel-details.jsx
Outdated
Show resolved
Hide resolved
79daaab
to
d2c7bb4
Compare
e515ecf
to
24b692b
Compare
d92717d
to
4528c87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 🙌🏽
I left some minor suggestions.
@@ -2,4 +2,4 @@ | |||
'@commercetools-frontend/application-components': patch | |||
--- | |||
|
|||
Accessibility improvements of the `TabHeader` component. | |||
Accessibility improvements of the `<TabHeader>` component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Why use the angle brackets here?
Do you mean <TabHeader />
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just my way of indicating that it's a React component, same as HTML elements are usually documented (e.g. <div>
, etc.)
Summary
The aim of this PR is to add a stater template in TypeScript.
Closes #2297
Ref: #1668