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

Button component #125

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

hayanisaid
Copy link
Contributor

  • Created Button component
  • Add onClick event

@hayanisaid hayanisaid requested a review from raphamorim as a code owner June 22, 2022 18:34
);
};

ctx.canvas.addEventListener('click', onClick, false);
Copy link
Owner

Choose a reason for hiding this comment

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

We need to remove addEventListeners from the renderButton function because this function runs for each state/prop update. It will keep creating/refreshing listeners for every render.

We can keep this way, if we run this addEventListener once by checking if the listener already exist. Note onClick will need to share scope with this function to work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on this!
Question! how to connect the component to the spatialGeometry context?

Copy link
Owner

Choose a reason for hiding this comment

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

I believe this property will need to be coming from parent View instead of itself. Btw, if you want to join the discord to talk, there's a link here https://medium.com/@raphamorim/bet-on-canvas-for-a-high-performant-tv-application-with-react-ape-cdf8c0b77c21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invalid Discord link invite!

Copy link
Owner

Choose a reason for hiding this comment

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

oops, try this link https://discord.gg/njHHfRzJ42

Copy link
Owner

@raphamorim raphamorim left a comment

Choose a reason for hiding this comment

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

Great job! Left some comments but it looks good for initial version

@raphamorim
Copy link
Owner

@hayanisaid other two points would be good to have in this PR:

  • Add usage information in the docs
  • Add an example of it working on the app folder

@raphamorim raphamorim mentioned this pull request Jun 24, 2022
@hayanisaid
Copy link
Contributor Author

@raphamorim Thanks for the feedback, working on applying those changes.

Copy link
Owner

@raphamorim raphamorim left a comment

Choose a reason for hiding this comment

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

Good job

import React from "react";
import { Button, View, render } from "react-ape";

class ImageExample extends React.Component {

Choose a reason for hiding this comment

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

Shouldn't this be ButtonExample?

title: string,
onPress: (event?: PressEvent) => mixed,
onClick: (event?: SyntheticMouseEvent<HTMLButtonElement>) => mixed,
//handleOnClick:(event?:SyntheticMouseEvent<HTMLButtonElement>)=>mixed,

Choose a reason for hiding this comment

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

is this dead code?

import {trackMousePosition, isMouseInside} from '../utils';
import type {CanvasComponentContext} from '../types';

//TODO adjust Opacity when focus, Blur

Choose a reason for hiding this comment

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

is there a ticket for this TODO?


// If is relative and x and y haven't be processed, don't render
// start drawing the canvas
console.log('[PROPS]', props);

Choose a reason for hiding this comment

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

do you want logging here?

const resetStyle = newStyle => {
globalStyle = {...globalStyle, newStyle};
};
// const redrawButton = ctx => {

Choose a reason for hiding this comment

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

can this be removed if commented out?

const newSize = `${ButtonDefaults.textStyle.fontSize}px`;
ctx.font = newSize + ' ' + fontArgs[fontArgs.length - 1];

// ctx.textAlign = 'center';

Choose a reason for hiding this comment

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

can this be removed if commented out?


// ctx.textAlign = 'center';

// ctx.fillText(title, 400 / 2, y + height / 2,textWidth);

Choose a reason for hiding this comment

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

can this be removed if commented out?

// ctx.fillText(title, 400 / 2, y + height / 2,textWidth);
ctx.fillText(title, x + textWidth / 2.5, y + height / 2);
ctx.closePath();
// if(props.handleOnClick){

Choose a reason for hiding this comment

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

can this be removed if commented out? This stretches all the way down to line 192

* Handling click button event
* @param {*} event
*/
// const onClick = (event: SyntheticMouseEvent<HTMLButtonElement>) => {

Choose a reason for hiding this comment

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

can this be removed if commented out?

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.

3 participants