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

Add language independent field icon. #6298

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

Conversation

iRohitSingh
Copy link
Contributor

Fix #6297

Copy link

netlify bot commented Sep 18, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 077102a
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66ed744f897a5200084d924b

Copy link
Sponsor Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@iRohitSingh Please take note of the checks which failed on this PR. It looks like you need to:

  1. Add the changelog
  2. Update test snapshots
  3. Update i18n

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Language and news look good. There are some failing tests to investigate.

@iRohitSingh
Copy link
Contributor Author

Language and news look good. There are some failing tests to investigate.

Ok. I am investigating it.

)}
>
<Grid>
<Grid.Row stretched>
{columns === 2 && (
<Grid.Column width="4">
<div className="wrapper">
<div className="wrapper title-icon-container">
Copy link
Sponsor Member

@davisagli davisagli Sep 19, 2024

Choose a reason for hiding this comment

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

Seeing all the snapshots that were updated makes me wonder:

  1. Do we need the title-icon-container class even for the fields that don't have an icon?
  2. Should we just apply the flex styles to all field wrappers and not add the new class?

/cc @sneridagh

Copy link
Member

Choose a reason for hiding this comment

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

I agree with David, although I can see that write a class enough specific to match only that wrapper can be difficult. @iRohitSingh can you look into that?

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.

Visually indicate that a field is a language independent field.
4 participants