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

Fix/semantic custom fields #1972

Closed
wants to merge 12 commits into from
Closed

Conversation

jacqueswho
Copy link
Contributor

@jacqueswho jacqueswho commented Aug 5, 2020

Reasons for making this change

Fix related to issue 1946

Features:

  • Semantic updated to 1.2.1
  • getSemanticProps to allow user pass semantic ui props including defaults
  • added submitButton
  • added Date and DateTime Widgets
  • added email Widget
  • getSemanticErrorProps to allow user to pass ui error props
  • implement getDisplayLabel from core/utils
  • added URLWidget

Fixes:

  • Semantic documentation updates
  • checkbox widget to use label or schema title
  • select widget to use label or schema title
  • textarea widget to use label or schema title
  • text widget to use label or schema title

If this is related to existing tickets, include links to them as well.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@jacqueswho jacqueswho added the semantic-ui semantic-ui related theme issue label Aug 5, 2020
@jacqueswho jacqueswho self-assigned this Aug 5, 2020
@jacqueswho jacqueswho requested a review from epicfaace August 5, 2020 20:00
@jacqueswho jacqueswho linked an issue Aug 12, 2020 that may be closed by this pull request
2 tasks
Copy link
Contributor Author

@jacqueswho jacqueswho left a comment

Choose a reason for hiding this comment

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

updated semantic
formContext props should over rider others

@jacqueswho
Copy link
Contributor Author

@epicfaace can I merge this in?

@epicfaace
Copy link
Member

epicfaace commented Dec 1, 2020

Sorry for the delay @jacqueswho. Could you update the PR description with a list of what features / changes exactly this PR adds? This PR seems pretty big and appears to do more than just fix #1946 (such as adding new widgets).

@jacqueswho
Copy link
Contributor Author

Sorry for the delay @jacqueswho. Could you update the PR description with a list of what features / changes exactly this PR adds? This PR seems pretty big and appears to do more than just fix #1946 (such as adding new widgets).

@epicfaace done

@jacqueswho
Copy link
Contributor Author

@epicfaace please check and approve if happy. What is the rule of thumb, do I merge after approval or the approver?

@jacqueswho
Copy link
Contributor Author

@epicfaace is there anything else?

@epicfaace
Copy link
Member

I'll take a look, sorry for the delay

@jacqueswho
Copy link
Contributor Author

@epicfaace is this project still being maintained? no activity

@epicfaace
Copy link
Member

Yes. @jacqueswho do you mind splitting this PR into separate PRs, one for each feature (for example, you could split it into one PR for the label / schema title fixes, one PR for the semantic update, one PR for each new widget)? That way, I'll be able to more easily tell which code fixes which issue, as it's not entirely clear from looking at this large, entire PR. The only PRs that make lots of unrelated changes should be the PRs that introduce a new theme (as that's unavoidable). That's the main reason I haven't been able to get to it, sorry for not letting you know about this earlier.

@jacqueswho
Copy link
Contributor Author

Yes. @jacqueswho do you mind splitting this PR into separate PRs, one for each feature (for example, you could split it into one PR for the label / schema title fixes, one PR for the semantic update, one PR for each new widget)? That way, I'll be able to more easily tell which code fixes which issue, as it's not entirely clear from looking at this large, entire PR. The only PRs that make lots of unrelated changes should be the PRs that introduce a new theme (as that's unavoidable). That's the main reason I haven't been able to get to it, sorry for not letting you know about this earlier.

@epicfaace this is going to be tricky, as the reason I included the new widgets with this pull request is I had to change the method getSemanticProps, which made me change how to pass the props in all the components, to follow the same approach. Its honestly going to be too much to change to make it separate. I have also been constantly waiting, and trying to merge with master to make sure it is up to date. Its almost been a month. I really am sorry, but I did not want to create separate pull requests to fix the other issues mentioned because of this change in every component how the props have been passed. Could we please just merge this in. In future the pull requests will be smaller. The main change this pull request, does is fix the issue of custom semantic props being passed properly. But I do think we should make this in core, and other themes just pass in their key that is related to finding the props

@epicfaace
Copy link
Member

epicfaace commented Feb 7, 2021

Sorry man, but I think splitting them up is the best way to ensure there aren't any regressions and that we agree on the substantial changes to the codebase that are made in this PR. Making changes in small and isolated PRs helps maintain the quality of the codebase for users of this library. Also, sometimes open source just takes a while (months / years) because people work for free 😉 . Totally get that you're probably tired of waiting for this PR to get reviewed, but I do want this PR to get in, so I'm happy to help with the work of splitting this up -- perhaps I can make the split-up PRs and then you can review them when they're ready?

I think the first thing I'd like to do is make a PR with just the new getSemanticProps and getSemanticErrorProps functions. However, it wasn't super clear to me what they are doing with the changes in this PR. Do you mind explaining a bit more what these functions are doing and how the new behavior differs from the old getSemanticProps function? Additionally, what's the rationale behind moving default props from declaring something like CheckboxWidget.defaultProps to defaultSchemaProps?

@jacqueswho
Copy link
Contributor Author

jacqueswho commented Feb 8, 2021

@epicfaace I have done as you asked except for the function change pr
#2226
#2225
#2224
#2227

regarding the function changes needed. default props does not work as intended, because if props are passed including the uiOptions with semantic props, it works fine. However if the user just adds one semantic prop, all the default props are lost. So I have changed the semantic functions to cater for this. If the user includes a new semantic prop that is not the default, they are not lost, but if they include on that is changing the defaults it will do so. So I feel all themes are going to have this issue, so we should maybe include this function in core utils. you pass the uiSchema and include your theme specific key and defaults

@epicfaace
Copy link
Member

@jacqueswho wow, thanks so much for doing this! I'll take a look.

@epicfaace
Copy link
Member

Closing this PR in favor of the PRs mentioned above that it's split up into.

@epicfaace epicfaace closed this Feb 14, 2021
@heath-freenome heath-freenome deleted the fix/semantic_custom_fields branch August 29, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semantic-ui semantic-ui related theme issue
Projects
None yet
3 participants