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

Prevents selecting a header that already is selected. #13

Merged
merged 5 commits into from
Jun 7, 2022

Conversation

RafaelSeSouza
Copy link

@RafaelSeSouza RafaelSeSouza commented May 31, 2022

Related to

N/A

Context

We should not add a header to a selection that already has that header selected.

Approach

  • Added a check on UserSelectionState.addSelection to prevent a selection to be added twice.
  • Removed the unused id property from the UserSelectionModel, as same selections can have different ids. We could remove the entire UserSelectionModel abstract class, and use Selection directly, but that would break the API.

@RafaelSeSouza RafaelSeSouza force-pushed the fix/add-existing-header-selection branch from 1084b99 to d6a66f1 Compare June 1, 2022 16:17
@RafaelSeSouza RafaelSeSouza changed the base branch from main to develop June 1, 2022 16:21
Copy link
Contributor

@bernardobelchior bernardobelchior left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
However, this will trigger a major version bump, since we're breaking the API.

Also, do we want to add the change to the CHANGELOG?

@RafaelSeSouza
Copy link
Author

The changes look good to me 👍 However, this will trigger a major version bump, since we're breaking the API.

Also, do we want to add the change to the CHANGELOG?

@bernardobelchior Done. Bumped to 1.2.0.

@@ -1,3 +1,7 @@
# 1.2.0

- Removes `id` property from `UserSelectionModel`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically a breaking change, although I doubt there is someone using this id.
I think for the time being we should mark this as deprecated and remove it from the API in a future major version (i.e., 2.0.0). What do you think?

Another option would be to release 2.0.0 already, but I think that would be too hasty. Specially since we have other PRs that would probably make sense to bundle in a new version release

Copy link
Author

Choose a reason for hiding this comment

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

I think you're right. Let me revert the changes and mark it as deprecated, and then we remove it on version 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 😄

@@ -13,6 +15,7 @@ const _uuid = Uuid();
/// Defines a [Selection] that is controllable by a [UserSelectionState].
abstract class UserSelectionModel extends Selection {
/// Unique identifier of a selection in a [UserSelectionState]
@Deprecated('')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a message? E.g., "id is deprecated and not used. It will be removed in the next major version"

Copy link
Author

Choose a reason for hiding this comment

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

I had a message like this, but leaving it blank already shows a similar default message.

Copy link

@victorbotamedi victorbotamedi left a comment

Choose a reason for hiding this comment

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

Well done.

We should probably discuss the deprecation policy and set a fixed number of major versions to remove the deprecated fields.

@RafaelSeSouza RafaelSeSouza force-pushed the fix/add-existing-header-selection branch from a638ad5 to 1f23d78 Compare June 7, 2022 17:34
@RafaelSeSouza RafaelSeSouza merged commit 4f22db8 into develop Jun 7, 2022
@RafaelSeSouza RafaelSeSouza deleted the fix/add-existing-header-selection branch June 7, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants