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

docs(ui-text-input,ui-view): functional examples are added to TextInput, View #1659

Conversation

Kadirsaglm
Copy link
Contributor

No description provided.

@matyasf matyasf requested review from matyasf and ToMESSKa August 29, 2024 15:29
@matyasf matyasf self-assigned this Aug 29, 2024
Comment on lines 213 to 215
onClick={function () {
console.log(this.state.value)
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This throws an error in the console: Uncaught TypeError: Cannot read properties of undefined (reading 'state').
I think if you convert this to an arrow function (like you did in the function based example) it will work fine. Could you please fix?
Feel free to rewrite the original class-based component if you notice an error. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this and i changed other anonymous functions to arrow functions to be more coherent

Comment on lines 481 to 482
const { isFocused, focusPosition } = this.state

Copy link
Contributor

Choose a reason for hiding this comment

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

Here these values are deconstructed but never used later. So i don't think this line is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Comment on lines 800 to 802
this.state = {
overflowY: 'visible'
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here only overFlowY is declared, but overFlowX is missing. Could you please add overFlowX too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ToMESSKa
Copy link
Contributor

Nice job. All of the recommended changes are in the original class-based examples.

@Kadirsaglm Kadirsaglm force-pushed the write_functional_examples_for_textinput_view branch from 94de2bf to d566483 Compare August 31, 2024 19:00
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

nice work!

@matyasf matyasf requested a review from ToMESSKa September 2, 2024 09:03
@matyasf matyasf merged commit 9abdfef into instructure:master Sep 6, 2024
10 of 11 checks passed
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