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

Prototype/Proposal for React FC extension, for creating custom versions for existing components - Vote if you find useful. #891

Closed

Conversation

surapuramakhil
Copy link
Contributor

Split of component, data, behavior favoring extensibility.

Code has been tested.

@surapuramakhil
Copy link
Contributor Author

@shuashuai @robinv8 request to review and merge this MR.

@shuashuai shuashuai self-requested a review April 7, 2024 06:31
@shuashuai
Copy link
Member

@surapuramakhil

Can you give an example of a place that can be expanded after such separation? Because a large hooks itself is difficult to maintain and cannot be reused; and doing so will also cause certain difficulties in reading the code.

It is true that for the early development of html + css + js, the principle of separation of performance, style, and behavior should be followed. However, since the development of frameworks, this is rarely done except when developing some component libraries.

@surapuramakhil
Copy link
Contributor Author

@shuashuai my goal here was to split view and viewModel/controller - Model being contract. This lets us create different variants of components (in this case its writeAnswer Component) it can be either having UI different / different internals (i.e. funtionality).

Because a large hooks itself is difficult to maintain and cannot be reused; and doing so will also cause certain difficulties in reading the code

code in webhook is existing code. I just moved code from FC to webhooks of the same component. No new code has been added. it's just refactoring.

@robinv8
Copy link
Contributor

robinv8 commented Apr 10, 2024

The separation of logic and views is reasonable, but excessive separation is also not conducive to component maintenance, especially when the logic cannot be reused and only serves one view.

In addition, there are standards for component development, but there are also project-specific standards. In this project, almost all components adhere to the following standards:

const Component = () => {
  // declare state
  const [state, setState] = useState({});

  useEffect(() => {

  }, []);

  // method
  const handleClick = () => {
    
  };

  return <div> Hello Answer</div>;
};

I think, Project Component Specification > Generic Component Specification

@surapuramakhil
Copy link
Contributor Author

@robinv8 separation is done for the purpose of reuse. it lets the site owner/admin tweak their site as per their requirements.

I think, Project Component Specification > Generic Component Specification

Honestly, I didn't get what you meant. I didn't understand what you mean by generic component specification

In addition, there are standards for component development, but there are also project-specific standards. In this project, almost all components adhere to the following standards.

can you share links for those docs

import { DRAFT_ANSWER_STORAGE_KEY } from '@/common/constants';
import { writeSettingStore } from '@/stores';

interface Props {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can reuse props defined at FC. I am not sure of the right approach. whether writeAnswer FC and webhook should have same props or can they be different etc.

I think. Having the same props with Union of prop requirements of both FC & webhook will be great.

@robinv8
Copy link
Contributor

robinv8 commented Apr 10, 2024

The common component specification I mentioned is just like the separation of logic and view as you said.

I think this PR excessively separates logic and views, and moves the original state of the component into hooks. Sorry, this PR cannot be merged.

@surapuramakhil
Copy link
Contributor Author

I think this PR excessively separates logic and views, and moves the original state of the component into hooks. Sorry, this PR cannot be merged.

The corrected version of this line is - this PR completely separates logic and views, and moves the original state of the component into its own hooks. Which means each component will be having FC which contains view and its own hook for state and component specific logic.

I understand Project Component Specification.

const Component = () => {
  // declare state
  const [state, setState] = useState({});

  useEffect(() => {

  }, []);

  // method
  const handleClick = () => {
    
  };

  return <div> Hello Answer</div>;
};

Can we have a proposal for modifying Project Component Specification? From current version to newer one. Any reasonably big component can have two files which defines component

  1. FC for View. FC fetches states from its hook
  2. its Webhook for storing its state and methods

FC

const Component = () => {
  // declare state
  const [data,methods] = useComponet({});

  return <div on-click=method> {data} </div>;
};

Hook

const useComponent = () => {
 // declare state
  const [state, setState] = useState({});

  useEffect(() => {

  }, []);

  // method
  const handleClick = () => {
    
  };
return { data: <All declared states>, methods: <declaread methods>}
}

Can we do this as a pilot and see if the new project structure doesn't add any additional load to maintainers?

@robinv8
Copy link
Contributor

robinv8 commented Apr 11, 2024

The state and methods are already part of the component and may not need to be changed for the time being. If you have other people's suggestions in the future, you can consider them. Making changes now would have too much impact

@surapuramakhil
Copy link
Contributor Author

surapuramakhil commented Apr 11, 2024

Making changes now would have too much impact

Ah, I don't mean changing all components to this right now. This can be done lazy On demand basics and New components can follow Project Component Specification

Impact on rest of the code

This restructure doesn't have any impact on other places. Just SOLID principles. There won't be any change in other components, which are using this FC as this web hook is called inside FC's and there is nothing that caller needs to be handled (composition code).

may not need to be changed for the time being

Reason why I have created this MR is to have a different variant of "write Answer" component which is more specific to my site. I value my time and your time too. And later in future even other components would require this.

Ah, remember these discussions? #826

@shuashuai @robinv8 @kumfo
if this is not satisfactory then just close this MR. We have discussed way more than it required.

@shuashuai
Copy link
Member

shuashuai commented Apr 11, 2024

@surapuramakhil

I roughly understand some of your thoughts. You hope that while the core functions remain unchanged, you can expand the functions needed for your business scenarios and avoid changing the core code; understood in this way, it can actually be regarded as an additional customization requirement. understood from this perspective, it can be seen as an additional customization requirement to facilitate users’ secondary development (that is, the component variant you want to implement).

From our point of view, very few users can truly achieve secondary development, and we don’t need to care about how users change and implement their own needs. We only need to ensure that the various functions provided by the official are normal and reasonable. enough.

As @robinv8 mentioned, we will only consider adopting it if it is what most people need, or if it is a common need!

@surapuramakhil surapuramakhil changed the title Write Answer refactor changers Prototype/Proposal for React FC extension, for creating custom versions for existing components - Vote if you find useful. Apr 21, 2024
@shuashuai shuashuai deleted the branch apache:dev May 6, 2024 07:31
@shuashuai shuashuai closed this May 6, 2024
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.

None yet

3 participants