feat: Added prfabs for checkbox 14px and 24px (MAPCO-9479)#29
feat: Added prfabs for checkbox 14px and 24px (MAPCO-9479)#29ofekazar wants to merge 1 commit intoMapColonies:masterfrom
Conversation
Assets/YahalomUIPackage/Editor/YahalomCheckbox/YahalomCheckboxEditor.cs
Outdated
Show resolved
Hide resolved
Assets/YahalomUIPackage/Runtime/YahalomCheckbox/YahalomCheckbox.cs
Outdated
Show resolved
Hide resolved
Assets/YahalomUIPackage/Editor/YahalomCheckbox/YahalomCheckboxEditor.cs
Outdated
Show resolved
Hide resolved
Assets/YahalomUIPackage/Editor/YahalomCheckbox/YahalomCheckboxEditor.cs
Outdated
Show resolved
Hide resolved
Assets/YahalomUIPackage/Editor/YahalomCheckbox/YahalomCheckboxEditor.cs
Outdated
Show resolved
Hide resolved
Assets/YahalomUIPackage/Editor/YahalomCheckbox/YahalomCheckboxEditor.cs
Outdated
Show resolved
Hide resolved
baruchInsert-tech
left a comment
baruchInsert-tech
left a comment
There was a problem hiding this comment.
- no need to have duplicate prefabs (24px and 14px), we can just change the size when needed - thats why i provided you only with the 72px pngs, also when exporting from Figma export without empty boundaries, if you have blank spaces then the prefab image wont fit the design
- remove all summary and comments in all files
- the current
_isPartialfield is a manually set serialized boolean, with a comment
saying it should eventually be replaced. here is how to implement it properly:
- add a
List<Toggle> _childTogglesserialized field to hold references to
child toggles. the_isPartialstate should be computed automatically — no longer
serialized. - in
OnEnable(), subscribe to each child'sonValueChanged. inOnDisable(),
unsubscribe. - when the parent is clicked: set all children to match the parent's
isOn
state (select all / deselect all). set_isPartial = false. - when any child changes: count how many children are on vs off.
-all on - parentisOn = true,_isPartial = false(checked)
-all off - parentisOn = false,_isPartial = false(unchecked)
- mixed -
_isPartial = true(partial) - use
SetIsOnWithoutNotify()to update the parent'sisOnwithout triggering
OnToggleChanged updateVisualState()sprite priority: disabled > partial > checked/unchecked.
|
@baruchInsert-tech When using the 4x pngs downsizing is done very poorly by unity. I couldn't get the settings quite right with that. |
have u tried to change Pixel Per Unit Multiplier on the image? |
|
@baruchInsert-tech Yes I tried to play with that value as well. the bottom one is the larger image. it is phsycally larger because it has no borders but you can see the artifacts even though it is larger. |
what size image are u using and what is the value u inserted? |
ofekazar
force-pushed
the
MAPCO-9479-ui-checkbox
branch
from
a72f809 to
9fe7872
Compare
ofekazar
force-pushed
the
MAPCO-9479-ui-checkbox
branch
from
9fe7872 to
f32e4fa
Compare
|
I tried all the values for 1 to 400. Anyway I got the logic of the children working recusivly in this patch and I remember to add the unboardered images this time. |
| protected override void OnValidate() | ||
| { | ||
| base.OnValidate(); | ||
|
|
||
| RecomputeFromChildren(); | ||
| UpdateVisualState(); | ||
| } |
There was a problem hiding this comment.
Not wrapped in #if UNITY_EDITOR (causes build warnings on non-editor)
calling RecomputeFromChildren() here is not needed - OnValidate is for editor preview only, UpdateVisualState() is enough
| private void HandleChildValueChanged(bool _) | ||
| { | ||
| RecomputeFromChildren(); | ||
| UpdateVisualState(); | ||
| PropagateStateToParent(); | ||
| } |
There was a problem hiding this comment.
add _isUpdating guard.
PropagateStateToParent() is unnecessary
UpdateVisualState() is already called inside RecomputeFromChildren
| private void PropagateToChildren(bool value) | ||
| { | ||
| for (int i = 0; i < _childToggles.Count; i++) { | ||
| Toggle child = _childToggles[i]; | ||
| if (child == null) { | ||
| continue; | ||
| } | ||
|
|
||
| if (child is YahalomCheckbox childCheckbox) { | ||
| childCheckbox.SetIsOnFromParent(value); | ||
| } | ||
| else { | ||
| child.isOn = value; | ||
| } | ||
| } | ||
|
|
||
| _isPartial = false; | ||
| } |
There was a problem hiding this comment.
Delete this method entirely. Its logic should inline inside OnParentValueChanged
|
|
||
| for (int i = 0; i < _childToggles.Count; i++) { | ||
| Toggle child = _childToggles[i]; | ||
| if (child == null || !child.IsInteractable()) { |
There was a problem hiding this comment.
Skipping non-interactable children makes the count unreliable. a disabled child that is checked should still count as checked.
| if (child is YahalomCheckbox childCheckbox && childCheckbox._isPartial) { | ||
| _isPartial = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Checking child._isPartial directly couples to the concrete type. with the simplified approach this is unnecessary a simple count of checked vs total children is enough to determine the parent's state.
| bool isInteractable = IsInteractable(); | ||
| if (isInteractable != _lastEffectiveInteractable) { | ||
| UpdateChildrenInteractable(isInteractable); | ||
| _lastEffectiveInteractable = isInteractable; | ||
| } |
There was a problem hiding this comment.
the interactable tracking and UpdateChildrenInteractable call doesn't belong here. a visual update method should only update visuals, not manage child state. Remove this block
| { | ||
| base.OnInspectorGUI(); | ||
|
|
||
| serializedObject.Update(); |
There was a problem hiding this comment.
serializedObject.Update() must come BEFORE base.OnInspectorGUI(), not after. When Update() is called after base, the serialized data shown in the base inspector may be stale for one frame.
baruchInsert-tech
left a comment
baruchInsert-tech
left a comment
There was a problem hiding this comment.
-
rename prefab to match other components, something like yahalomcheckbox and not just checkbox.
-
rename ui assets to match other components, "toggle_check"
or "toggle_disabled" , not just numbers -
you have imported the exact size of the assets and not 4x. - please import the 4x size of the assets. it should solve the issues with quality
asafmas-rnd
changed the title
2 participants
No description provided.