Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Added blur event to select-field #11

Merged
merged 9 commits into from
Feb 20, 2019

Conversation

blackbaud-conorwright
Copy link
Contributor

@blackbaud-conorwright blackbaud-conorwright commented Feb 5, 2019

Issue: #10
Docs: blackbaud/skyux2-docs#228

I expect there may be some concerns about the name blur and I can change it. I felt that it is what that event means and this component doesn't have it, so it seemed right. But I also realize that we may just want to completely avoid "reserved" event names.

Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

@blackbaud-conorwright got one comment for you.

Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

@blackbaud-conorwright this hooks to where we fire "blur" on "touch". In the datepicker we fire "touch" on "blur" of the input. I'm concerned that with this implementation we will fire the "blur" event on initial touch and not on actually blurring the component. Have we verified this?

@blackbaud-conorwright
Copy link
Contributor Author

Hmm, that's true. This could be a good reason to rename the event to something like touch instead. This will be firing blur when the user closes the modal or the user changes the value, neither of which guarantees that the field was blurred/lost focus. How do you feel about renaming the event to touch or something else explicit to a touched event?

@blackbaud-conorwright
Copy link
Contributor Author

^ @Blackbaud-TrevorBurch
sorry forgot to @ you ;p

@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #11 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
+ Coverage   99.43%   99.47%   +0.03%     
==========================================
  Files           6        6              
  Lines         178      190      +12     
  Branches       24       26       +2     
==========================================
+ Hits          177      189      +12     
  Partials        1        1
Impacted Files Coverage Δ
...lic/modules/select-field/select-field.component.ts 99.06% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dae8df...88b21eb. Read the comment docs.

@Blackbaud-TrevorBurch
Copy link
Member

@blackbaud-conorwright sounds good to me. I should go with touched or something that uses the past tense as that falls more in line with other events that we fire in our components. @Blackbaud-SteveBrush might have some opinions but is out on PTO today.

@blackbaud-conorwright
Copy link
Contributor Author

@Blackbaud-SteveBrush and @Blackbaud-TrevorBurch I've updated the name to be touched

Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

One question

@blackbaud-conorwright
Copy link
Contributor Author

@Blackbaud-TrevorBurch and @Blackbaud-SteveBrush I've updated this to be a "true touch" blur event

@Blackbaud-SteveBrush Blackbaud-SteveBrush self-assigned this Feb 20, 2019
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

Just two very minor things.

@blackbaud-conorwright
Copy link
Contributor Author

@Blackbaud-TrevorBurch I believe I've resolved both comments 👍

@blackbaud-conorwright
Copy link
Contributor Author

and @Blackbaud-SteveBrush Your review was dismissed by my last commit ;p

Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

LGTM

@Blackbaud-SteveBrush Blackbaud-SteveBrush merged commit 422395f into master Feb 20, 2019
@Blackbaud-SteveBrush Blackbaud-SteveBrush deleted the select-field-blur-event branch February 20, 2019 20:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants