-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add screenshot button #4
Conversation
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.
Het canvas heeft als default achtergrond kleur wit op de webpagina. Ik neem aan dat dit is omdat het achterliggende component wit is. Echter als je een screenshot maakt is de achtergrond zwart, wit of transparant afhankelijk van de image viewer die je gebruikt. Ik neem aan dat de oorzaak hiervan is dat het canvas zelf transparant is. Ik weet ook niet of we dit op moeten lossen aangezien we bij het renderen van de visualisaties altijd eerst het canvas clearen met een default kleur, dus ik neem aan dat dit dan ook geen issue meer is.
Als er niks op het canvas staat dan levert het maken van een screenshot een onleesbaar bestand op. Dit is zeker en edge case, maar willen we deze handelen?
Erg snel geïmplementeerd trouwens :)
public capture(): void { | ||
const dataUrl: string = this.canvas.toDataURL('image/' + ScreenshotButtonComponent.FILE_FORMAT); | ||
|
||
// create temporary link to download |
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.
Dit lijkt mij nogal een omslachtige manier om iets te downloaden, is het standaard voor web sites om tijdelijk een link toe te voegen aan de pagina om een download dialoog te maken? (het werkt prima dus daar verder niks op aan te merken, ik ben gewoon benieuwd).
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.
Helemaal mee eens dat dit omslachtig is! Maar op dit moment wel de enige helaas. Er zijn geen browser-wide file api's beschikbaar, en dit is op dit moment helaas de enige manier.
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.
Een goeie inderdaad voor het canvas! Ik denk dat het een non-issue is, gezien we wellicht ook een landingspage maken voor er iets getekend is. Anders zou een tijdelijke oplossing zijn om tijdens de initialisatie 1x de canvas al te clearen met wit o.i.d.
En thanks :)
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.
Ah, dus het gebrek aan een API is de oorzaak. En inderdaad laten we de canvas edge cases voor nu maar even met rust laten en deze pas later oplossen als ze tegen die tijd nog relevant zijn.
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.
Misschien edge cases als issue toevoegen zodat we eventueel in de toekomst dit nog kunnen fixen/het niet vergeten :)
@teards issue is aangemaakt. Zou je deze, en #3 kunnen reviewen? |
Ja is prima, ik ze al bekeken maar had niet gereviewed vanwege mijn onervarenheid nog met de taal. Dus kleine nuances ontgaan mij waarschijnlijk nog. Vond het er, to my knowledge, goed uit zien dus zal ze even reviewen. |
1a6f31a
No description provided.