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

Create traffic sign component #848

Merged
merged 21 commits into from
May 3, 2024
Merged

Conversation

AronPetrauskas
Copy link
Contributor

@AronPetrauskas AronPetrauskas commented Apr 25, 2024

Changes

A new RUI component is introduced that renders traffic signs onto a konva canvas.

  • TrafficSign component created.
  • TrafficSign component variations created in storybook.
  • End-to-end tests written for the TrafficSign component.

Dependencies

UI/UX

Highway traffic sign example:
image

Testing notes

Important

Failed CI tests are a known issue, please ignore. Tests failing because of "timeout" are a known flaky test issue.

  1. Checkout the branch
  2. pnpm i
  3. pnpm run storybook
  4. Test the 'TrafficSign' component under Road View (category)
    Test all props can be changed and the component updates as expected.

image

Author checklist before assigning a reviewer

  • Reviewed my own code-diff.
  • [ ] Branch has been run in docker.
  • PR assigned to me or an appropriate delegate.
  • Relevant labels added to the PR.
  • Appropriate tests have been added.
  • Lint and test workflows pass.

@AronPetrauskas AronPetrauskas self-assigned this Apr 25, 2024
@AronPetrauskas AronPetrauskas mentioned this pull request Apr 25, 2024
6 tasks
@AronPetrauskas AronPetrauskas added the enhancement New feature or request label Apr 30, 2024
@dmbarry86
Copy link
Contributor

5. Using the testing tab in VSCode run the tests manually. Make sure they all run. Make sure storybook is running before testing!

Note

You'll have to run the test at least twice. First run generates the reference images. Make sure the reference images display the expected component.

I don't think we should recommend manually running tests in PRs like this. You should recommend to run the tests via Docker, for which snapshots already exist. Don't confuse it for reviewers.

@dmbarry86 dmbarry86 self-requested a review May 1, 2024 07:24
@dmbarry86
Copy link
Contributor

A new RUI component is introduced that renders traffic signs onto a konva canvas.

  • TrafficSign component created.
  • TrafficSign component variations created in storybook.
  • End-to-end tests written for the TrafficSign component.

It would be nice to show a screenshot of the component in future reviews. That's what the UI section is for (which you deleted by the look of it).

@dmbarry86
Copy link
Contributor

You need to make sure your component is exported from the root index otherwise you won't be able to import it in your app. Same goes for figure component which also appears to be missing although already merged.

https://github.com/IPG-Automotive-UK/react-ui/blob/create-traffic-sign-component/src/index.ts

package-lock.json Outdated Show resolved Hide resolved
@dmbarry86
Copy link
Contributor

The default story should show default values for all props. The "set number" buttons etc mean there isn't currently a value defined in the story. Make sure all stories have appropriate values defined for all props.

image

@dmbarry86
Copy link
Contributor

In future, just delete sections that are no longer relevant rather than strikethrough. I don't need to see the past state when I'm reviewing it today.

image

Copy link
Contributor

@dmbarry86 dmbarry86 left a comment

Choose a reason for hiding this comment

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

I've had an initial run through and bash. Some general and some specific comments. I've told the team to hold off reviewing the other PRs for now as some of the general points here will also come up in those PRs so it seems best to get one right first and then ask people to review the others in which case hopefully they are then just picking up on specific things for that component. I appreciate some of the general points are because you are new to the team. I've asked the senior developers to start a code style guideline document so maybe you could also contribute to that with the things you have picked up so far with your time in the team which we don't yet have written down. @jegasriskantha is going to start that doc.

@AronPetrauskas AronPetrauskas requested a review from dmbarry86 May 2, 2024 13:40
@dmbarry86
Copy link
Contributor

dmbarry86 commented May 3, 2024

I don't think this is related to your changes but there's something weird with storybook giving me this error for your component and all components. If I open in incognito browser it works fine but doesn't work in my usual browser window. Some weird cache or something maybe between branches. I'm copying in @mattcorner to take a look as I wonder if it's related to vite changes #810.

image

No need to hold up this PR with it.

@AronPetrauskas AronPetrauskas merged commit fe52e4a into main May 3, 2024
1 check failed
@AronPetrauskas AronPetrauskas deleted the create-traffic-sign-component branch May 3, 2024 13:57
syedsalehinipg added a commit that referenced this pull request May 9, 2024
syedsalehinipg added a commit that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants