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/ocrvs 7978/qr reader #8196

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from
Open

Conversation

tahmidrahman-dsi
Copy link
Collaborator

No description provided.

@tahmidrahman-dsi
Copy link
Collaborator Author

/uikit

@naftis naftis marked this pull request as ready for review December 12, 2024 08:09
@naftis
Copy link
Collaborator

naftis commented Dec 12, 2024

/uikit

@ocrvs-bot
Copy link
Collaborator

Storybook deployed: https://feat-ocrvs-7978-qr-reader.opencrvs.pages.dev

Comment on lines +24 to +43
display: flex;
align-items: center;
width: ${width || '100%'};

:before, :after {
flex: 1;
content: '';
padding: 0 1px 1px;
background-color: ${color || theme.colors.grey200};
margin: 16px;
}

:before {
margin-left: 0;
}

:after {
margin-right: 0;
}
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically what does this do if Divider has children? Could this be documented in Storybook 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep sure! I introduced children prop to the divider to make components like this below. As it serves the same purpose of a divider component, I made the prop change in the divider component

image

Comment on lines 32 to 44
const renderReader = (reader: ReaderType) => {
const { type, ...readerProps } = reader
if (type === 'qr') {
return (
<QRReader
key={type}
{...readerProps}
onScan={onScan}
onError={onError}
/>
)
} else return null
}
Copy link
Collaborator

@naftis naftis Dec 16, 2024

Choose a reason for hiding this comment

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

This is a "ReaderGenerator" (like FormFieldGenerator), can be lifted to a separate component so it doesn't get re-created on every render of IDReader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I am on it 👍🏼

<Icon name={props.iconName} size="large" />
</IconContainer>
<Text variant="reg14" element="p">
{props.label}
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
{props.label}
{children}

Comment on lines 23 to 25
const StyledButton = styled(SecondaryButton)`
width: 100%;
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Button component instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thanks. will do

Copy link

Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:

  • Changelog is read by country implementors who might not always be familiar with all technical details of OpenCRVS. Keep language high-level, user friendly and avoid technical references to internals.
  • Answer "What's new?", "Why was the change made?" and "Why should I care?" for each change.
  • If it's a breaking change, include a migration guide answering "What do I need to do to upgrade?".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants