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

Arrayed Configs support #40

Merged
merged 18 commits into from
Apr 6, 2022
Merged

Arrayed Configs support #40

merged 18 commits into from
Apr 6, 2022

Conversation

Senna46
Copy link
Collaborator

@Senna46 Senna46 commented Apr 5, 2022

No description provided.

@Senna46
Copy link
Collaborator Author

Senna46 commented Apr 5, 2022

Status Report

  • Support for Configs list
  • Implement Change Node pull-down list

To Do

Switch to selected node
(Now, not async, log only )
スクリーンショット 2022-04-05 194208

@Senna46 Senna46 changed the title feat: configs Arrayed Configs support Apr 5, 2022
Senna46 added a commit that referenced this pull request Apr 6, 2022
@Senna46
Copy link
Collaborator Author

Senna46 commented Apr 6, 2022

@YasunoriMATSUOKA
Implemented async node switching.
Is it safe to proceed with the work as is? Please confirm.

2c8f1e1
The node to switch to is flushed as BehaviorSubject and made Observable in config.service.
Each component uses this Observable to async use the value of config.
スクリーンショット 2022-04-06 151650
スクリーンショット 2022-04-06 151703

@YasunoriMATSUOKA
Copy link
Contributor

I will check it now, wait a moment plz 🙏

@YasunoriMATSUOKA
Copy link
Contributor

YasunoriMATSUOKA commented Apr 6, 2022

Great works!
The implementation seems to be LGTM!

This is a matter of preference, but I think configTypeOptions-like name would be good for configOptions or configs.
I think it would be less confusing to use configType as config too.

Thank you!

@Senna46
Copy link
Collaborator Author

Senna46 commented Apr 6, 2022

@YasunoriMATSUOKA
Debug will be put in Issue. Please confirm if it's okay to merge.

8f1e519
Supports Config selection

e432d5f
.subscribe was used in this place and I has been fixed. @kimurayu45z has confirmed.

Others are support config$ (Observable)

Copy link
Contributor

@YasunoriMATSUOKA YasunoriMATSUOKA left a comment

Choose a reason for hiding this comment

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

This is a matter of preference, but I think configTypeOptions-like name would be good for configOptions or configs.
I think it would be less confusing to use configType as config too.

Did you check that?
I think it's not a type so it should be changd...

@Senna46
Copy link
Collaborator Author

Senna46 commented Apr 6, 2022

Changes are complete.
I debugged the config switching only on the top page.
Other pages cannot be judged immediately. It has been confirmed that the values flow when switching config.

Copy link
Contributor

@YasunoriMATSUOKA YasunoriMATSUOKA left a comment

Choose a reason for hiding this comment

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

LGTM

@YasunoriMATSUOKA YasunoriMATSUOKA merged commit 6ca099e into develop Apr 6, 2022
@YasunoriMATSUOKA YasunoriMATSUOKA deleted the feature/configs branch April 6, 2022 11:18
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.

2 participants