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

Refactor drawer domain #996

Merged
merged 12 commits into from
Dec 2, 2020
Merged

Refactor drawer domain #996

merged 12 commits into from
Dec 2, 2020

Conversation

yong-jie
Copy link
Member

@yong-jie yong-jie commented Dec 1, 2020

Problem

Drawer domain was implemented and migrated in a way that unnecessarily exposed a lot of implementation. There was a lot of code reimplementation that could have been abstracted for reuse.

Partially fixes #970

Solution

Improvements:

Also moved state tracking of edited longURl into LongUrlEditor
component instead of redux
Throughout all uses of ConfigOption, the title variant and title
classnames follow a certain pattern. For mobile responsive fields,
they start with h6 variant and use regularText CSS when in mobile view

Given that this pattern has repeated for many times, move the
implementation into ConfigOption's implementation and expose a boolean
isMobileResponsive flag to toggle the behaviour.
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

generally lgtm.

@yong-jie yong-jie merged commit 77ea2d6 into develop Dec 2, 2020
@yong-jie yong-jie deleted the refactor-drawer branch December 2, 2020 03:25
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.

File error message not cleared when navigating to other file links in drawer
2 participants