-
Notifications
You must be signed in to change notification settings - Fork 328
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
feat: Allow alignment for ui.text #1370 #2336
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.
Good job @dbranley! A few minor comments and good to go.
ui/src/text.test.tsx
Outdated
textAlignPropValues.forEach(textAlignPropValue => { | ||
rerender(<XText {...textProps} align={textAlignPropValue} />) | ||
expect(queryByTestId(name)).toBeInTheDocument() | ||
expect(queryByTestId(name)).toHaveAttribute('style','text-align: '+textAlignPropValue+';') |
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.
TLDR: This unit test can be removed.
No need to unit test visual things as they are very coupled to the implementation - will break on refactor. We have visual regression tests for this scenario that can be used to verify no other components changed unexpectedly.
If you are interested, you can checkout to main, run make build-ui
to build the UI, then cd tools/showcase
and make visual-regression
. If you have never run it before, it will create base snapshots that will be used for comparison in the next run. Now checkout your feature branch, build UI again and run visual regression tests again. It parses all the examples in documentation and compares against your baseline (main). It's super strict so false positives are very likely, but still good enough to quickly see if something broke or not. One more thing - if new examples are inserted (like it is now in text.md
), the order will not match and you will get a lot of mismatches. In that case, you just check if the screenshots look "normal" since there is no way around that.
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.
@mturoci Thanks for explaining this about the visual regression tests, I hadn't noticed it yet! I ran through this and think this is a good testing tool. I'll be sure to run this as a check before submitting any UI changes in the future.
py/examples/text_alignment.py
Outdated
ui.separator('Align - default'), | ||
ui.text(text), | ||
ui.separator('Align - start'), | ||
ui.text(text, align=ui.TextAlign.START), | ||
ui.separator('Align - end'), | ||
ui.text(text, align=ui.TextAlign.END), | ||
ui.separator('Align - center'), | ||
ui.text(text, align=ui.TextAlign.CENTER), | ||
ui.separator('Align - justify'), | ||
ui.text(text, align=ui.TextAlign.JUSTIFY), | ||
ui.separator('Various sizes - align center'), | ||
ui.text_xl('Extra large text', align=ui.TextXlAlign.CENTER), | ||
ui.text_l('Large text', align=ui.TextLAlign.CENTER), | ||
ui.text_m('Medium text', align=ui.TextMAlign.CENTER), | ||
ui.text_s('Small text', align=ui.TextSAlign.CENTER), | ||
ui.text_xs('Extra small text', align=ui.TextXsAlign.CENTER), |
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.
No need to also include sizes, showing different alignments with default size is enough. The more concise the examples the better.
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.
@mturoci Done! Here's a screen shot of the new example:
…oved text size examples, and fixed issues so new examples render on website h2oai#1370
@mturoci Thanks for all the great feedback! I think I've take care of all the points, but please let me know if I missed something or you notice something else. 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.
Perfect!
Closes #1370
The PR fulfills these requirements: (check all that apply)
main
branch.feat: Add a button #xxx
, where "xxx" is the issue number).Closes #xxx
, where "xxx" is the issue number.ui
folder, unit tests (make test
) still pass.As requested in the ticket, I have added a new
align
prop intext.tsx
. The possible values arestart
,end
,center
, andjustify
, with a default ofstart
. This new prop is then passed down through thestyle
prop forFluent.Text
. The code change inui/src/text.tsx
looks like this:I then ran the make generate target to update the generated python and R code to reflect this new attribute in files, such as
py/h2o_wave/h2o_wave/ui.py
.I have updated the documentation in
website/widgets/form/text.md
with a new section calledAlignment
. I ran the make targets to regenerate the documentation artifacts and you can see a screen shot of that here:I tested this with a new custom python example that uses the new
align
attribute available forui.text
. You can find this atpy/examples/text_alignment.py
. And here is a screen shot of the new example:I created a new unit test in
text.test.tsx
, where it tests each of the possible values for the new prop, along with confirming it defaults tostart
. Here is a snipit from the test:I ran all the ui tests and confirmed they passed. Here is a screen shot for that: