-
Notifications
You must be signed in to change notification settings - Fork 75
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
Create a new Studio Component named RecommendedNextAction #13296
Create a new Studio Component named RecommendedNextAction #13296
Conversation
209ec13
to
7aa9cd7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13296 +/- ##
==========================================
+ Coverage 93.73% 93.74% +0.01%
==========================================
Files 1438 1446 +8
Lines 19937 20029 +92
Branches 2420 2428 +8
==========================================
+ Hits 18688 18777 +89
- Misses 1002 1004 +2
- Partials 247 248 +1 ☔ View full report in Codecov by Sentry. |
8f5a185
to
16afefa
Compare
16afefa
to
1412130
Compare
Maybe a use case is a button where the user just approves or declines a suggested change?
…commendednextaction
…commendednextaction
…commendednextaction
…commendednextaction
It's not the exact same, but very similar: #bfc4cb vs #bcbfc5
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.
Great work! Just a few small comments on some of the tests. 😊
...o-components/src/components/StudioRecommendedNextAction/StudioRecommendedNextAction.test.tsx
Outdated
Show resolved
Hide resolved
...c/components/StudioRecommendedNextAction/context/StudioRecommendedNextActionContext.test.tsx
Show resolved
Hide resolved
...ts/ConfigPanel/ConfigContent/EditTaskRecommendedActions/RecommendedActionChangeName.test.tsx
Outdated
Show resolved
Hide resolved
...ts/ConfigPanel/ConfigContent/EditTaskRecommendedActions/RecommendedActionChangeName.test.tsx
Outdated
Show resolved
Hide resolved
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.
👏
…commendednextaction
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.
Testing
Nice feature and implementation 👏
@Annikenkbrathen: Nitpicking, but I noticed that the font sizing is different between the normal view and RecommendedNextAction. It's not crucial by any means, but it would maybe look a little bit more cohesive if the sizing was the same?
@Jondyr: It is not possible to save the new ID by pressing enter. If it is not a quick fix, maybe it can be a separate issue.
But overall, it looks and feels like a solid component 😄
Spiller.inn.2024-08-21.092443.mp4
Agreed, well observed. I still think the texts here are one size too large. Are you using small? It should be 15px. And the header should be x-small, 21px. Is there a reason you moved away from the background color under the component? I also notice that this field no longer takes up the full width. Is this part of the same issue, or does it belong elsewhere |
This is now implemented. 👍
This was a misunderstanding on my part, the paragraph is set to small now. The header is x-small already and should be 21px (for some reason it's actually 20.96 px).
I am confused how this happened, but this was not intended and most likely happened when i commited the code changes for the changes discussed here. This is now fixed.
Good catch, this issue was introduced when refactoring the component and a css class was lost. This is now fixed as well. |
Very nice @Jondyr! One thing I noticed in the code is the use of fireEvent in some of the tests. Can you replace them with userEvent? I think it technically does not matter which one we use, but we are using userEvent elsewhere in the code base, and should be consistent with it 😊 |
I noticed the header for the whole panel in the Figma sketch is 24px, while on master/staging it's 18px. That might contribute to the feeling that the rest of the fonts are the wrong size? 🤔 I can possibly change this to match the figma sketch size, if preferred? @Annikenkbrathen Also the text looks slightly off-centered compared to the icon on staging to me, maybe this can be solved at the same time. Unless this is part of another task? |
I agree, userEvent is what i would have preferred to use. But it seems userEvent does not support forms, as i got Update: - fireEvent.submit(screen.getByRole('button', { name: 'Save' }));
+ await userEvent.click(screen.getByRole('button', { name: 'Save' })); jsdom throws this error:
There are lots of suggested solutions in the jsdom github issue 1937. (Not linking it to avoid polluting cross reference list in their issue) |
…commendednextaction
Description
RecommendedNextAction
StudioRecommendedNextActionContext
to track recommendation suggestionsStudioRecommendedNextActionTest1.mp4
Changes to existing code:
EditTaskId.tsx
->useValidateBpmnTaskId.tsx
)Related Issue(s)
Verification
Documentation