incubator-answer icon indicating copy to clipboard operation
incubator-answer copied to clipboard

Write Answer refactor changers

Open surapuramakhil opened this issue 10 months ago • 10 comments

Split of component, data, behavior favoring extensibility.

Code has been tested.

surapuramakhil avatar Apr 03 '24 23:04 surapuramakhil

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

surapuramakhil avatar Apr 04 '24 11:04 surapuramakhil

@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.

shuashuai avatar Apr 10 '24 03:04 shuashuai

@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.

surapuramakhil avatar Apr 10 '24 03:04 surapuramakhil

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

robinv8 avatar Apr 10 '24 04:04 robinv8

@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

surapuramakhil avatar Apr 10 '24 04:04 surapuramakhil

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.

robinv8 avatar Apr 10 '24 06:04 robinv8

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?

surapuramakhil avatar Apr 10 '24 12:04 surapuramakhil

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

robinv8 avatar Apr 11 '24 02:04 robinv8

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? https://github.com/apache/incubator-answer/discussions/826

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

surapuramakhil avatar Apr 11 '24 03:04 surapuramakhil

@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!

shuashuai avatar Apr 11 '24 07:04 shuashuai