Skip to content

fix-next(css): className to preserve root views classes#7725

Merged
vchimev merged 7 commits intoreleasefrom
vchimev/class-name
Aug 30, 2019
Merged

fix-next(css): className to preserve root views classes#7725
vchimev merged 7 commits intoreleasefrom
vchimev/class-name

Conversation

@vchimev
Copy link
Copy Markdown
Contributor

@vchimev vchimev commented Aug 23, 2019

PR Checklist

What is the current behavior?

When you set className to a (modal) root view, it overwrites view's default CSS classes.

What is the new behavior?

When you set className to a (modal) root view, it preserves view's default CSS classes.

Fixes #7709.

const shouldAddModalRootViewCssClass = cssClasses.has(MODAL_ROOT_VIEW_CSS_CLASS);
const shouldAddRootViewCssClasses = cssClasses.has(ROOT_VIEW_CSS_CLASSES[0]);

cssClasses.clear();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we clear this at all? Maybe modifying the add method to not add a cssClass if it already exists would skip all of this adding and removing classes?

Also modifying cssClassses should update className.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually just saw that cssClasses is a Set, so it should have only unique items, thus no need for even checking on add(). 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, deleting all the new code and cssClasses.clear() works okay.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now we are left with only one problem - all the new root classes are not reflected in className, thus the user can't see them in the string or in DevTools.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, since cssClasses is not synced with className, it has to be cleared every time before adding classes, to remove any classes that are not in className...

@@ -1036,11 +1038,23 @@ bindingContextProperty.register(ViewBase);
export const classNameProperty = new Property<ViewBase, string>({
name: "className",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So as discussed offline with @vchimev and @bundyo currently there is a discrepancy between className and cssClasses -- if you try to get the className value it will not include any values that were added through cssClasses directly. One potential solution could be to update the className getter to return a concatenated string of cssClasses values.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We will log a separate issue for this as apparently we will need to expose public API for customizing property getter/setter.

@vchimev vchimev force-pushed the vchimev/class-name branch from ad42320 to 557d34b Compare August 28, 2019 19:25
@vchimev vchimev requested a review from manoldonev August 28, 2019 19:25
@vchimev vchimev force-pushed the vchimev/class-name branch from 557d34b to dfdd748 Compare August 28, 2019 19:52
@vchimev vchimev force-pushed the vchimev/class-name branch from c7651f3 to 8b912be Compare August 29, 2019 14:30
@vchimev vchimev merged commit d23ffb8 into release Aug 30, 2019
@vchimev vchimev deleted the vchimev/class-name branch August 30, 2019 06:09
manoldonev added a commit that referenced this pull request Sep 12, 2019
* feat(android): fix tab resource icon size based on spec (#7737)

* feat(ios): add icon rendering mode for bottom navigation (#7738)

* fix(ios-tabs): crash when add tabstrip in loaded event (#7743)

* fix(css): parse css selectors with escape sequences (#7689) (#7732)

* fix(ios-tabs): handle nesting proxy view container (#7755)

* fix-next(css): className to preserve root views classes (#7725)

* docs: cut the 6.1.0 release (#7773)

* fix(android-list-picker): NoSuchFieldException on api29 (#7790)

* chore: hardcode tslib version to 1.10.0 (#7776)

* fix(css-calc): reduce_css_calc_1.default is not a function (#7787) (#7801)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants