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

fix(protocol-designer): fix ListItemDescriptor content and description alignment issue #16588

Merged
merged 4 commits into from
Oct 24, 2024

Conversation

koji
Copy link
Contributor

@koji koji commented Oct 24, 2024

Overview

fix ListItemDescriptor content and description alignment issue

remove string type from description and content.
There are 2 types in the design, but actually we need to use ListDescriptor in SlotInfo that has 118.5px as width.
Technically we can add a condition to switch 3 types but at this moment we don't know how Desktop app and other web application use it. For this change, we might need to write a little bit more lines, but it makes things easy since we can pass JSX.Element that we style or layout separately.

design
https://www.figma.com/design/WbkiUyU8VhtKz0JSuIFA45/Feature%3A-Protocol-Designer-Phase-1?node-id=10092-334055&m=dev

SlotDetails

before

Screenshot 2024-10-23 at 6 20 23 PM

after

Screenshot 2024-10-23 at 7 07 54 PM

Protocol Overview

Protocol Metadata

Instruments

Liquid Definitions

Protocol Steps

Materials List

Screenshot 2024-10-23 at 8 06 59 PM

Screenshot 2024-10-23 at 8 07 12 PM

close RQA-3424

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

koji added 3 commits October 23, 2024 19:06
…n alignment issue

fix ListItemDescriptor content and description alignment issue

close RQA-3424
@koji koji requested review from ncdiehl11 and jerader October 24, 2024 00:17
Comment on lines +269 to +272
minWidth="13.75rem"
alignItems={ALIGN_CENTER}
gridGap={SPACING.spacing8}
width="13.75rem"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Liquid is a special case since the description is a user input and we don't know the width of description.

) : (
''
)
<Flex minWidth="13.75rem">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

13.75rem is Mel's request. ListItemDesciptor's description always keep 220px.

@koji koji marked this pull request as ready for review October 24, 2024 00:33
@koji koji requested review from a team as code owners October 24, 2024 00:33
@koji koji removed request for a team October 24, 2024 03:41
Comment on lines +14 to 15
import type { FC } from 'react'
import { FLEX_ROBOT_TYPE } from '@opentrons/shared-data'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import type { FC } from 'react'
import { FLEX_ROBOT_TYPE } from '@opentrons/shared-data'
import { FLEX_ROBOT_TYPE } from '@opentrons/shared-data'
import type { FC } from 'react'

description={t('robotType')}
description={
<Flex minWidth="13.75rem">
{' '}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{' '}

content={pipetteInfo(leftPipette)}
description={
<Flex minWidth="13.75rem">
{' '}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{' '}

content={pipetteInfo(rightPipette)}
description={
<Flex minWidth="13.75rem">
{' '}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{' '}

content={isGripperAttached ? t('gripper') : t('na')}
description={
<Flex minWidth="13.75rem">
{' '}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{' '}

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.

lgtm thanks for the quick fix! I left some minor clean up comments but might merge this in around 9 so the list item descriptor can be fixed for candidate B

@jerader jerader merged commit 5c04eab into edge Oct 24, 2024
28 checks passed
@jerader jerader deleted the fix_RQA-3424 branch October 24, 2024 12:41
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.

2 participants