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

refactor(ui5-li): selected property changed to private #9246

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

yanaminkova
Copy link
Member

The selected property of the ui5-li has been redefined within the class where it is actively utilized. Despite this change, it remains accessible to all components that rely on it.

Related to #8461, #7887

Copy link
Contributor

@nnaydenow nnaydenow left a comment

Choose a reason for hiding this comment

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

add declare keyword in front of the places where we only want to change the documentation

@nnaydenow
Copy link
Contributor

FYI @MapTo0

@dobrinyonkov
Copy link
Contributor

Related to #9022

@yanaminkova
Copy link
Member Author

add declare keyword in front of the places where we only want to change the documentation

We only need to update the documentation for the ui5-li-notification and ui5-li-notification-group components to remove the selected property. The documentation for all other components should remain unchanged.
Is the declare statement still required in this scenario?

@tsanislavgatev
Copy link
Contributor

@dobrinyonkov @yanaminkova
We've merged: #8871
We inherit ListItemBase and document the selected property as unusable in our case.
Can you please delete our documentation of the selected property in the MenuSeparator component, as the only reason to document it is because of the inheritance?

@dobrinyonkov dobrinyonkov requested a review from kineticjs June 24, 2024 12:16
Copy link
Contributor

@kineticjs kineticjs left a comment

Choose a reason for hiding this comment

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

So my understanding is that we define the "select" property in the base class only (i.e in ListItemBase) and then declare it as inherited property in the subclasses where it makes sense to use it.. In the remaining places (MenuSeparator) no mentioning (i.e. no declaration/definition) of the "select" property. Seems coherent, so approving...

@kineticjs kineticjs requested a review from nnaydenow June 24, 2024 14:40
@nnaydenow nnaydenow dismissed their stale review June 25, 2024 12:37

Review addressed

@dobrinyonkov dobrinyonkov merged commit e53301d into main Jun 25, 2024
10 checks passed
@ilhan007 ilhan007 deleted the ui5-li-selected branch June 25, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants