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

feat(protocol-designer): update icons for steps #16335

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

syao1226
Copy link
Collaborator

@syao1226 syao1226 commented Sep 23, 2024

re AUTH-799

Overview

Update icons for steps in the Protocol timeline to match the design.

Test Plan and Hands on Testing

  • Ensure all icons match the design

Changelog

  • added new icons: ot-move, transfer, mix, ot-start, and ot-end to the icon-data.ts file
  • added a condition to display icons for Starting deck state and Ending deck state

Review requests

Risk assessment

@syao1226 syao1226 requested review from jerader and koji September 24, 2024 14:11
@syao1226 syao1226 marked this pull request as ready for review September 24, 2024 14:29
@syao1226 syao1226 requested review from a team as code owners September 24, 2024 14:29
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

looks great! just left a comment

transfer: {
path:
'M3.33333 12.1673C3.33333 10.5423 3.89931 9.16384 5.03125 8.0319C6.16319 6.89996 7.54167 6.33398 9.16667 6.33398C10.6528 6.33398 11.9479 6.82357 13.0521 7.80273C14.1562 8.7819 14.7917 10.0076 14.9583 11.4798L16.3333 10.1673L17.5 11.334L14.1667 14.6673L10.8333 11.334L12.0208 10.1673L13.25 11.3965C13.0556 10.4104 12.5764 9.59787 11.8125 8.95898C11.0486 8.3201 10.1667 8.00065 9.16667 8.00065C8.01389 8.00065 7.03125 8.4069 6.21875 9.2194C5.40625 10.0319 5 11.0145 5 12.1673L5 14.6673L3.33333 14.6673L3.33333 12.1673Z',
viewBox: '0 0 17 18',
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the 20x20 one i exported and i feel like it's a little small:Screenshot 2024-09-24 at 12 35 50 PM and this is the adjusted 17x18:
Screenshot 2024-09-24 at 12 37 02 PM

Copy link
Contributor

@koji koji Sep 24, 2024

Choose a reason for hiding this comment

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

@jerader what do you think about 👆?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no preference - this is a design decision i guess! Lets ask Mel and Felix

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, using the same size is easy for us because we can change the actual icon size in a component and no icon's position/spacing issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that makes sense, i'll keep the 20x20 as it is.

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

Looks good to merge to me 👍 thanks for unifying the viewBox size!

@syao1226 syao1226 merged commit 31fd73e into edge Sep 26, 2024
48 checks passed
@koji
Copy link
Contributor

koji commented Sep 26, 2024

@syao1226 thanks!

TamarZanzouri pushed a commit that referenced this pull request Sep 26, 2024
re AUTH-799

<!--
Thanks for taking the time to open a Pull Request (PR)! Please make sure
you've read the "Opening Pull Requests" section of our Contributing
Guide:


https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests

GitHub provides robust markdown to format your PR. Links, diagrams,
pictures, and videos along with text formatting make it possible to
create a rich and informative PR. For more information on GitHub
markdown, see:


https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax

To ensure your code is reviewed quickly and thoroughly, please fill out
the sections below to the best of your ability!
-->

# Overview

<!--
Describe your PR at a high level. State acceptance criteria and how this
PR fits into other work. Link issues, PRs, and other relevant resources.
-->
Update icons for steps in the Protocol timeline to match the design.

## Test Plan and Hands on Testing

<!--
Describe your testing of the PR. Emphasize testing not reflected in the
code. Attach protocols, logs, screenshots and any other assets that
support your testing.
-->

- Ensure all icons match the
[design](https://www.figma.com/design/WbkiUyU8VhtKz0JSuIFA45/Feature%3A-Protocol-Designer-Phase-1?node-id=2171-26665&node-type=canvas&t=ja8Z7s8Rdk6yeO8K-0)

## Changelog

<!--
List changes introduced by this PR considering future developers and the
end user. Give careful thought and clear documentation to breaking
changes.
-->

- added new icons: `ot-move`, `transfer`, `mix`, `ot-start`, and
`ot-end` to the `icon-data.ts` file
- added a condition to display icons for Starting deck state and Ending
deck state
## Review requests

<!--
- What do you need from reviewers to feel confident this PR is ready to
merge?
- Ask questions.
-->

## Risk assessment

<!--
- Indicate the level of attention this PR needs.
- Provide context to guide reviewers.
- Discuss trade-offs, coupling, and side effects.
- Look for the possibility, even if you think it's small, that your
change may affect some other part of the system.
- For instance, changing return tip behavior may also change the
behavior of labware calibration.
- How do your unit tests and on hands on testing mitigate this PR's
risks and the risk of future regressions?
- Especially in high risk PRs, explain how you know your testing is
enough.
-->

---------

Co-authored-by: shiyaochen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants