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

Character count (#59) #61

Merged
merged 11 commits into from
Sep 23, 2019
Merged

Character count (#59) #61

merged 11 commits into from
Sep 23, 2019

Conversation

Blackbaud-BobbyEarl
Copy link

Merging so we don't lose @Blackbaud-JackMcElhinney's work. Original at #59

  • initial

  • mostly functional

  • redesign

  • fixture refactor

  • unit tests

  • cleanup and visual tests

@Blackbaud-JackMcElhinney
Character count (#59)
a78048c 
* initial

* mostly functional

* redesign

* fixture refactor

* unit tests

* cleanup and visual tests
@codecov
Copy link

codecov bot commented Aug 14, 2019 •
edited
Loading

Codecov Report

Merging #61 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #61   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     23    +3     
  Lines         792    878   +86     
  Branches      110    120   +10     
=====================================
+ Hits          792    878   +86
Impacted Files Coverage Δ
...s/character-counter/character-counter.directive.ts 100% <100%> (ø)
...ules/character-counter/character-counter.module.ts 100% <100%> (ø)
...r-counter/character-counter-indicator.component.ts 100% <100%> (ø)

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 0ae8204...4ba5efb. Read the comment docs.

Blackbaud-SteveBrush
Blackbaud-SteveBrush reviewed Aug 27, 2019
View reviewed changes
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.

Some initial feedback.

One thing: the form control should be invalidated on keyup (not just blur).

src/app/public/modules/character-counter/character-counter-indicator.component.html Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter-indicator.component.html Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter-indicator.component.scss Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter-indicator.component.ts Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter.directive.ts Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter.directive.ts Outdated Show resolved Hide resolved
Blackbaud-AlexKingman
Blackbaud-AlexKingman reviewed Aug 28, 2019
View reviewed changes
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Just a couple comments. Looking good!

Blackbaud-SteveBrush
Blackbaud-SteveBrush reviewed Sep 4, 2019
View reviewed changes
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.

Just a few minor things!

Something unrelated to your work, but if we could fix the NPM vulnerability warnings in this PR, that would be ideal (run npm install to see them).

src/app/public/modules/character-counter/character-counter-indicator.component.html Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter-indicator.component.ts Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter-indicator.component.ts Show resolved Hide resolved
src/app/index.html Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter-indicator.component.ts Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter.module.ts Show resolved Hide resolved
src/app/public/modules/character-counter/index.ts Outdated Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter.module.ts Show resolved Hide resolved
src/app/public/modules/character-counter/character-counter.directive.ts Outdated Show resolved Hide resolved
Blackbaud-ToddRoberts
Blackbaud-ToddRoberts reviewed Sep 17, 2019
View reviewed changes
Copy link

@Blackbaud-ToddRoberts Blackbaud-ToddRoberts left a comment

Choose a reason for hiding this comment

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

looks good to me and screen reader functionality when over the limit is nice

Blackbaud-AlexKingman
Blackbaud-AlexKingman previously approved these changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

As far as I'm concerned, this looks great!

@Blackbaud-ErikaMcVey Blackbaud-ErikaMcVey merged commit 210604a into master Sep 23, 2019
@Blackbaud-ErikaMcVey Blackbaud-ErikaMcVey deleted the contrib-character-count branch September 23, 2019 17:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Blackbaud-ToddRoberts Blackbaud-ToddRoberts Blackbaud-ToddRoberts left review comments

@Blackbaud-ErikaMcVey Blackbaud-ErikaMcVey Blackbaud-ErikaMcVey left review comments

@Blackbaud-SteveBrush Blackbaud-SteveBrush Blackbaud-SteveBrush approved these changes

@Blackbaud-AlexKingman Blackbaud-AlexKingman Blackbaud-AlexKingman left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Blackbaud-BobbyEarl @Blackbaud-SteveBrush @Blackbaud-ToddRoberts @Blackbaud-AlexKingman @Blackbaud-ErikaMcVey @Blackbaud-JackMcElhinney