-
Notifications
You must be signed in to change notification settings - Fork 116
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
New: Draw annotations UI #403
Conversation
src/lib/PreviewUI.js
Outdated
*/ | ||
replaceHeader(replacementHeader) { | ||
// First hide all possible headers | ||
this.container.querySelectorAll(`.${CLASS_HIDDEN}`).forEach((element) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to add CLASS_HIDDEN
to all elements that have CLASS_HIDDEN
?
Shouldn't this just be selecting the header element and hiding it, if it exists?
src/lib/annotations/Annotator.scss
Outdated
right: 10px; | ||
} | ||
|
||
.bp-btn-annotate-draw-undo, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you don't have :hover, :active, and :focus?
I'm just curious as to why those are included
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to override the normal button styles since they assume things like background/border for all of these states. The undo redo buttons are an svg in a clear button. I considered not using `'bp-btn' but we gain convenience with the disabled state, cursor, and using the correct hit box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonyjin have you dealt with anything like this before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using 'bp-btn-plain`?
I'd do something like class="bp-btn bp-btn-plain bp-btn-annotate-draw-undo"
- not sure if you need both bp-btn
and bp-btn-plain
.replace(/\W*(\w)\w*/g, '$1') | ||
.toUpperCase() | ||
.substring(0, 3); | ||
initials = userName.replace(/\W*(\w)\w*/g, '$1').toUpperCase().substring(0, 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for record, we're planning on moving away from initials, and moving towards colours and symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was prettier, I didn't touch this line.
Tests incoming