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

1276: Update sdsTableRowNavigation directive #1278

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

cwolf10
Copy link
Collaborator

@cwolf10 cwolf10 commented Aug 3, 2023

Description

Note: Adding breaking-change label for visibility. This PR does not contain any breaking changes, but does lay the groundwork for breaking changes in the future. All changes in the PR are just to the internal workings of table-row

This PR lays the groundwork to require highlighting when rowClick event is enabled. The purpose of doing this is to align with Angular Material in how they handle this exact scenario. Currently highlighting is an optional directive and rowClick is always emitting. However, aligning with Material, rows should be highlighted if they are going to emit an event.

In this PR I:

  • Update the class name, selector, and file name and location of this directive.
  • Add a new input to the directive which more closely aligns with the purpose of this directive. This input will become the primary input.
  • Add a warning when rowClick event emits and neither input is used, warning devs that option to emit without highlighting is going away and to use emitOnRowClick input to ensure event continues emitting in the future.
  • Add warning if highlightOnHover input is used. This input will be going away and emitOnRowClick should be used in it's place.
  • Move rowClick event emit logic to directive in order to force warning messages. With event emission being tied to the input of this directive, it also makes sense to live here.
  • Update rowConfig to include new input.
  • Add comment to note what should be removed in future updates

Motivation and Context

#1276. See above for rationale for this change.

Type of Change (Select One and Apply Github Label)

  • Bug fix (non-breaking change which fixes an issue) -> Apply bugfix label
  • New feature (non-breaking change which adds functionality) -> Apply enhancement label
  • Breaking change (fix or feature that would cause existing functionality to change) -> Apply breaking label

Screenshots (if appropriate):

Which browsers have you tested?

  • Internet Explorer 11
  • Edge
  • Chrome
  • Firefox
  • Safari

Checklist:

@cwolf10 cwolf10 added the breaking Tag PR as a breaking change label Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Tag PR as a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants