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

Add height attribute to ui.copyable_text if multiline is True #1308

Closed
mturoci opened this issue Mar 24, 2022 Discussed in #1302 · 6 comments · Fixed by #1694
Closed

Add height attribute to ui.copyable_text if multiline is True #1308

mturoci opened this issue Mar 24, 2022 Discussed in #1302 · 6 comments · Fixed by #1694
Labels
feature Feature request good first issue Contributions welcome! ui Related to UI

Comments

@mturoci
Copy link
Collaborator

mturoci commented Mar 24, 2022

Discussed in #1302

Originally posted by azim-b March 22, 2022
can we add a height param to the ui.copyable_text() component?

Example use case:

  • text box of certain height
  • user enters text and hits Submit
  • if the query was successfully, user gets back a "copyable text box" of same height (i.e. ui.copyable_text() of same height)

Description

Allow controlling height of the ui.copyable_text if multiline is set to True. Should work the same as ui.textbox.

@mturoci mturoci added ui Related to UI feature Feature request good first issue Contributions welcome! labels Mar 24, 2022
@scharf-roie
Copy link

Hey @mturoci, can I take this issue? I haven't worked with UI specifically for python before but I am experienced in the language. Any additional guidelines would be very helpful!

@mturoci
Copy link
Collaborator Author

mturoci commented Sep 22, 2022

Hi @scharf-roie, definitely go for it! However, note that the work needed to be done is going to be in React/Typescript mostly - https://github.com/h2oai/wave/blob/master/ui/src/copyable_text.tsx.

Please go through our contributing guide and set up your dev env first. Should you have any questions, do not hesitate to ask on our Discord :)

@mturoci
Copy link
Collaborator Author

mturoci commented Oct 3, 2022

Since 4 days have passed already, the issue is up for grabs again.

@cvanvoorhis
Copy link
Contributor

Hi @mturoci, I am a student looking to contribute to Wave. This task seems like a good starting point, do you think I could take this issue?

I propose a simple change to ui.copyable_text.jsx, wherein we add a height property to the copyable_text interface, add height to the model, and change this line:
<Fluent.TextField componentRef={ref} value={value} label={label} multiline={multiline} styles={{ root: { width: pc(100) } }} readOnly />
to this line:
<Fluent.TextField componentRef={ref} value={value} label={label} multiline={multiline} styles={ multiline ? { root: { width: pc(100), height: height} } : { root: { width: pc(100) } } } readOnly />

Also, I plan to add test case(s) for this, but I'm not sure I understand the current tests just by looking at them. Is there some resource for writing TypeScript tests for Wave that you could point me to?

@mturoci
Copy link
Collaborator Author

mturoci commented Nov 11, 2022

Hi @cvanvoorhis, please go ahead and create a PR.

not sure I understand the current tests

Wave has 2 types of tests currently:

  • Unit tests (*.test.tsx) - these test the component interactions and business logic. Your proposed change doesn't do either of these.
  • Visual tests - screenshot px by px comparison for visual changes - this one is the way to go for this task. All you need to do is add a working example into copyable_text.md.

@cvanvoorhis
Copy link
Contributor

cvanvoorhis commented Nov 12, 2022

Thank you @mturoci! Been a little busy so I'm planning to get a PR up tomorrow when I have more time.

mturoci added a commit that referenced this issue Dec 12, 2022
Co-authored-by: Martin Turoci <martin.turoci@h2o.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request good first issue Contributions welcome! ui Related to UI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants