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

WIP: feat(Settings): added settings layout comp #77

Closed
wants to merge 3 commits into from

Conversation

TanyaDolgopolova
Copy link
Contributor

Fixed design on general settings
Close #16
Close #55

Fixed design on general settings
@todo
Copy link

todo bot commented Oct 30, 2020

add list component of option views

// TODO: add list component of option views
// TODO: add children components for option view. Do it need to be in kti project?
// TODO: make methods optionally
const LayoutSettings: React.FC<LayoutSettingsProps> = ({ tenant = {} }) => {
const handleThemeChoose = (
event: React.FormEvent<Element>,


This comment was generated by todo based on a TODO comment in ab06f82 in #77. cc @tabetalt.

@todo
Copy link

todo bot commented Oct 30, 2020

add children components for option view. Do it need to be in kti project?

// TODO: add children components for option view. Do it need to be in kti project?
// TODO: make methods optionally
const LayoutSettings: React.FC<LayoutSettingsProps> = ({ tenant = {} }) => {
const handleThemeChoose = (
event: React.FormEvent<Element>,
done: () => void


This comment was generated by todo based on a TODO comment in ab06f82 in #77. cc @tabetalt.

@todo
Copy link

todo bot commented Oct 30, 2020

make methods optionally

// TODO: make methods optionally
const LayoutSettings: React.FC<LayoutSettingsProps> = ({ tenant = {} }) => {
const handleThemeChoose = (
event: React.FormEvent<Element>,
done: () => void
) => {


This comment was generated by todo based on a TODO comment in ab06f82 in #77. cc @tabetalt.

@kvokov kvokov marked this pull request as ready for review November 3, 2020 13:18
@kvokov
Copy link
Contributor

kvokov commented Nov 3, 2020

👍

Copy link
Member

@simenandre simenandre left a comment

Choose a reason for hiding this comment

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

Left a few comments, but this looks good to me :)

@@ -35,14 +42,27 @@ const Header: React.FC<HeaderProps> = ({ children, links = [] }) => {
to={link.to}
key={i}
activeStyle={{
color: '#1D1D1D',
Copy link
Member

Choose a reason for hiding this comment

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

Should this be added to theme.colors maybe? Maybe you can decide, @AnneMatilde?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace the color #1D1D1D with #1B202E.

Comment on lines +119 to +146
<OptionView
completedText="Valgt"
contentName="IBM Plex Sans + IBM Plex Serif"
loadingText="Bruk"
onPreviewClick={handleFontPreview}
onSelectClick={handleFontChoose}
previewText="Forhåndsvis"
>
<></>
</OptionView>
<OptionView
completedText="Valgt"
contentName="IBM Plex Sans + IBM Plex Serif"
loadingText="Bruk"
onPreviewClick={handleFontPreview}
onSelectClick={handleFontChoose}
previewText="Forhåndsvis"
>
<></>
</OptionView>
<OptionView
completedText="Valgt"
contentName="Work Sans + Roboto"
loadingText="Bruk"
onPreviewClick={handleFontPreview}
onSelectClick={handleFontChoose}
previewText="Forhåndsvis"
>
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a list that we can pull for the API. For now, we can add it as an array, but later @dem1k can add that to the backend, what do you say?

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 think the best option for this is create list component in Kit project, like we did with checkboxlist. I thinking about create an array which I will pass in parameters as a variant

@simenandre
Copy link
Member

@TanyaDolgopolova You can merge stuff on your own, when you've got the reviews you need :)

@AnneMatilde
Copy link
Contributor

Hi @TanyaDolgopolova! Have you finished with the design implementation for this?
image

@TanyaDolgopolova
Copy link
Contributor Author

Hi @TanyaDolgopolova! Have you finished with the design implementation for this?
image

This is still in progress. I will add this list of views to the kit project and connect it to this component. For now I left it like that

@simenandre
Copy link
Member

This is still WIP, correct? Can you add a WIP: syntax while it is?

@TanyaDolgopolova
Copy link
Contributor Author

This is still WIP, correct? Can you add a WIP: syntax while it is?

It still in progress. I will add it

@TanyaDolgopolova TanyaDolgopolova changed the title feat(Settings): added settings layout comp WIP: feat(Settings): added settings layout comp Nov 6, 2020
@TanyaDolgopolova TanyaDolgopolova changed the title WIP: feat(Settings): added settings layout comp feat(Settings): WIP added settings layout comp Nov 6, 2020
@simenandre simenandre changed the title feat(Settings): WIP added settings layout comp WIP: feat(Settings): added settings layout comp Nov 6, 2020
@simenandre
Copy link
Member

This has become stale, closing it. Reopen if need be. 👍

@simenandre simenandre closed this Dec 28, 2020
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.

Tenant Settings Implement LayoutSettings
4 participants