Skip to content

Conversation

@azech-hqs
Copy link
Contributor

No description provided.

l[k].classNames = v.classNames
? `${v.classNames} ${this.defaultClassNames}`
: this.defaultClassNames;
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what happened here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, it would add undefined to the default class name if the UI schema did not have a classNames property for the given field. I just changed the logic to only add classNames to the string if it is actually defined.

While working on this, I found the behavior of this function a bit counterintuitive, and perhaps we want to consider changing it so that whenever classNames is present in the UI schema, it overwrites rather than prepends the default class names.

Copy link
Member

Choose a reason for hiding this comment

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

  1. The above could be done with just:
l[k].classNames = `${v.classNames || ''} ${this.defaultClassNames || ''}`;
  1. It seems that you changes the logic for ...this.defaultFieldStyles also - is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding 2, defaultFieldStyles already contains classNames and it seemed redundant, but I will change it back so that one may implement a custom version locally (which is not currently the case).

@timurbazhirov timurbazhirov merged commit 2f1e2ac into main Mar 14, 2023
@timurbazhirov timurbazhirov deleted the chore/SOF-1549 branch March 14, 2023 02:17
k0stik pushed a commit that referenced this pull request Jul 31, 2023
chore/SOF-1549: Enable user to specify target k-point density per 1/Angstrom
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