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

💄 Change position of info icon to error display for mobile view #619

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sasamuku
Copy link
Member

@sasamuku sasamuku commented Jan 29, 2025

Summary

Changed position of InfoIcon on ErrorDisplay.

The placement of the InfoIcon has been implemented in a tricky way.
Please let me know if there is a better way to meet design.

Before

🎨 Design 💻 ParseError 💻 NetworkError
image image image

After

🎨 Design 💻 ParseError 💻 NetworkError
image image image

Related Issue

Changes

Testing

Other Information

Copy link

changeset-bot bot commented Jan 29, 2025

🦋 Changeset detected

Latest commit: 114a688

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@liam-hq/erd-core Patch
@liam-hq/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@MH4GF
Copy link
Member

MH4GF commented Jan 30, 2025

@sasamuku Am I correct in my understanding that the title and text should be aligned at the start on PC, but not on SP?

PC
Screenshot_2025-01-30_at_11_39_48

SP
Screenshot_2025-01-30_at_11_40_38

You are right, it is complicated.
It would have been more beneficial not to overcomplicate things here, so it seemed better to check with the our designer about her intentions and change it simple!

@hoshinotsuyoshi
Copy link
Member

hoshinotsuyoshi commented Jan 30, 2025

Oh, my bad. 🙇‍♂️ It probably started around #446 or #502, but I just noticed it after seeing this comment.

In browsers other than Chrome (e.g., Safari), the title message appears to be using a serif font-family.

I believe using a non-serif font is correct. Could you confirm? 🙏 @sasamuku

@sasamuku
Copy link
Member Author

@MH4GF

Am I correct in my understanding that the title and text should be aligned at the start on PC, but not on SP?

Right, I considered changing a component structure, but it was difficult.

it seemed better to check with the our designer about her intentions and change it simple!

Sounds nice👍 I'll check with designer.

@sasamuku
Copy link
Member Author

sasamuku commented Jan 30, 2025

@hoshinotsuyoshi

I believe using a non-serif font is correct. Could you confirm?

Thanks for letting me know! I'll check it.

Fixed by:

@sasamuku sasamuku mentioned this pull request Jan 30, 2025
@sasamuku
Copy link
Member Author

sasamuku commented Jan 31, 2025

Finally

InfoIcon is hidden on mobile to avoid complexity:

💻 ParseError 💻 NetworkError
image image

@MH4GF Could you please re-review?

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