Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

Change remove click event oc icon #2216

Merged
merged 4 commits into from
Jun 22, 2022
Merged

Change remove click event oc icon #2216

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e61e2a
Remove unused StatusIndicator component import in OcResource
pascalwengerter May 24, 2022
28ad273
Remove onclick event for OcIon
pascalwengerter Jun 22, 2022
5e9751a
Update changelog item for removed click event on OcIcon
pascalwengerter Jun 22, 2022
4ed2b7a
Add necessary maxwidth to OcNotification class
pascalwengerter Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/unreleased/change-remove-click-event-oc-icon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Change: Remove click event on OcIcon
Copy link
Member

@kulmann kulmann Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMO not a change, but an enhancement, maybe even just a bugfix. Before this PR the click handler on ocIcon would be used like this:

<oc-icon @click="someCallback" />

Since the click handler was living on the root component of the template, writing the code above would still emit a click event like before, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can still add an event handler like the someCallback example you provided. In theory it's a change because someone somewhere out there might have something listening on an OcIcon emitting a click event, but can re-scope the changelog item if necessary

Copy link
Member

Choose a reason for hiding this comment

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

But the only way to listen to that click event is by adding a click handler like in the example above. Or could also be that I'm just missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I am? 🤔 let me change it to a bugfix changelog then


We have removed the default click handler on the OcIcon component, expecting it to increase performance of the UI.
Adding a custom click-event handling on an instance of the component is of course still possible - albeit then wrapping it in an OcButton is probably the right choice from an accessibility standpoint.

https://github.com/owncloud/owncloud-design-system/pull/2216
4 changes: 0 additions & 4 deletions src/components/atoms/OcIcon/OcIcon.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
sizeClass(size),
variationClass(variation),
]"
@click="onClick"
>
<inline-svg
:src="nameWithFillType"
Expand Down Expand Up @@ -169,9 +168,6 @@ export default {
prefix(string) {
if (string !== null) return `oc-icon-${string}`
},
onClick() {
this.$emit("click")
},
transformSvgElement(svg) {
if (this.accessibleLabel !== "") {
const title = document.createElement("title")
Expand Down
2 changes: 0 additions & 2 deletions src/components/organisms/OcResource/OcResource.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
<script>
import OcButton from "../../atoms/OcButton/OcButton.vue"
import OcImg from "../../atoms/OcImage/OcImage.vue"
import OcStatusIndicators from "../../molecules/OcStatusIndicators/OcStatusIndicators.vue"
import OcIcon from "../../atoms/OcIcon/OcIcon.vue"
import OcResourceName from "../../atoms/OcResourceName/OcResourceName.vue"
import OcResourceIcon from "../../atoms/OcResourceIcon/OcResourceIcon.vue"
Expand All @@ -78,7 +77,6 @@ export default {
components: {
OcButton,
OcImg,
OcStatusIndicators,
OcIcon,
OcResourceName,
OcResourceIcon,
Expand Down