semantic-ui: Use getDisplayLabel to properly show labels for widgets #2225
Conversation
|
@agustin107 ping |
|
Hello @agustin107 @epicfaace @jacqueswho any news on this pull request? |
sorry @ErQrYfkrju cant do anything. just waiting. |
There was a problem hiding this comment.
Hey @jacqueswho , just a few things that we need before getting this PR ready:
- can you remove the extraneous changes that aren't really relevant for this change? That will ensure this PR is narrowly scoped to only include the get display util-related changes. For example, changes in props destructuring, extra blank lines deleted / added, etc.
- let's add some tests with the different schemas / uischemas with labels to ensure that all the cases here are tested by these tests so that they don't regress in the future.
|
@epicfaace have made all the changes. except please keep the props approach, as the in other pull requests it is like this, and most of the other themes , use this approach to make it easier for applying validation and functions for all props |
There was a problem hiding this comment.
Thanks, can you update the snapshots so the tests pass @jacqueswho ?
Hi @epicfaace if you look at the files changed I already did. and the tests pass for me, but my version is @rjsf/semantic-ui@2.5.0 but not sure why the tests are running 2.5.1? Do I need to merge with master and then try again? |
|
@jacqueswho hmm, I wonder if you actually need to revert the snapshot changes in this PR. If you run |
|
@epicfaace I ran that command and no changes to push? |
jacqueswho
force-pushed
the
feature/use_get_display_label_util
branch
2 times, most recently
from
b30d1d5
to
68ffdb3
Compare
feat: use core/util getDisplayLabel Revert changes of other packages Revert "feat: add button,email,date widgets" This reverts commit 5c99def. dep: @rjsf/core changed back feat: remove getDisplayLabel checkbox and select widget feat: removed core util changes feat: update checkbox widget feat: undo import order chore: tests updated
jacqueswho
force-pushed
the
feature/use_get_display_label_util
branch
from
68ffdb3
to
3faee3e
Compare
|
@epicfaace please can this be merged |
There was a problem hiding this comment.
Sorry @jacqueswho for the delay, only now getting a better chance to look at this. In addition to the changes I mentioned, can you also add a simple snapshot test that checks that #1946 is working for a simple example?
| return ( | ||
| <Form.Dropdown | ||
| key={id} | ||
| name={name} | ||
| label={label || schema.title} |
There was a problem hiding this comment.
Shouldn't we use getDisplayLabel here as well?
There was a problem hiding this comment.
I don't think so, as I have seen all other themes not do this? must be a reason why, unless you see otherwise I can do the same
There was a problem hiding this comment.
It's probably a mistake, I think all widgets should be using getDisplayLabel even in other themes. For now, though, let's just leave it as is for consistency.
epicfaace
changed the title
Reasons for making this change
Features:
#1946
Checklist