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

Profile Detail View UI #753

Merged
merged 27 commits into from
Apr 18, 2020
Merged

Profile Detail View UI #753

merged 27 commits into from
Apr 18, 2020

Conversation

meghanacosmos
Copy link
Contributor

@meghanacosmos meghanacosmos commented Apr 8, 2020

Added the UI of Profile detail view, issue: #743.

Ignitus (1)

@@ -0,0 +1,22 @@
import React from 'react';
import {HTMLAttributes} from 'react';
type Props = HTMLAttributes<SVGElement>;
Copy link

Choose a reason for hiding this comment

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

Parsing error: Unexpected token Props

@@ -0,0 +1,22 @@
import React from 'react';
import {HTMLAttributes} from 'react';
type Props = HTMLAttributes<SVGElement>;
Copy link

Choose a reason for hiding this comment

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

Parsing error: Unexpected token Props

Copy link
Member

@Paarmita Paarmita left a comment

Choose a reason for hiding this comment

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

@meghana-12 Can you please add a screenshot of your UI in the PR description.

@@ -0,0 +1,195 @@
import React from 'react';
import { StyledHeading2, StyledHeading4, StyledHeading6 } from '../../../ignitus-UserInterfaceBook/styles';
import * as C from '../styles';
Copy link
Member

Choose a reason for hiding this comment

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

We use C generally for importing colors, can you please user P for Profile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!

`;

export const Container2 = styled.div`
margin-right: -0.3rem;
Copy link
Member

Choose a reason for hiding this comment

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

Don't use margin in negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

`;

export const HeadingContainer = styled(flexibleRowDiv)`
// padding-top: 3rem;
Copy link
Member

Choose a reason for hiding this comment

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

Remove comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

flex-wrap: wrap;
`;

export const ContriElement = styled(flexibleRowDiv)`
Copy link
Member

Choose a reason for hiding this comment

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

Please try to use understandable convention names instead of Contri

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll edit that

@meghanacosmos meghanacosmos requested a review from Paarmita April 8, 2020 13:51
@Paarmita Paarmita linked an issue Apr 10, 2020 that may be closed by this pull request
Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

A visual representation of flex layout for recommendation section, layout is not that clean but will give you hints how you should use flex.

Screenshot 2020-04-10 at 20 59 41

One tutorial that will certainly help you to better use flex is https://flexboxfroggy.com/

@divyanshu-rawat
Copy link
Member

@meghana-12 please remove the .png image added as well, and use the avatar I suggested., please delete the Image.

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #753 into develop will decrease coverage by 0.00952%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##            develop       #753         +/-   ##
=================================================
- Coverage   0.56818%   0.55866%   -0.00951%     
=================================================
  Files             5          6          +1     
  Lines           176        179          +3     
  Branches         48         48                 
=================================================
  Hits              1          1                 
- Misses          175        178          +3     
Impacted Files Coverage Δ
...c/ignitus-Routes/ignitus-DashboardRoutes/index.jsx 0.00000% <0.00000%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0d2f05...c8b96d5. Read the comment docs.

@@ -0,0 +1,33 @@
import {AppIcon} from '../../ignitus-Shared/types/iconsTypes/iconEnums';

export type HeadingProps = {
Copy link

Choose a reason for hiding this comment

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

Parsing error: Unexpected token type

<RoundedButton size="medium" category="grey">Psycholinguistics </RoundedButton>
<RoundedButton size="medium" category="grey">Modernist Literature </RoundedButton>
</P.ContentContainer>
</P.ElementContainer>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
</P.ElementContainer>
</P. ResearchSection >

import {Paragraph} from '../../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/typography';
import {Loading} from '../../../ignitus-Shared/ignitus-Utilities/Components/loader';
import {Normal} from '../../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/fonts';
import {default as I} from '../../../ignitus-Shared/ignitus-Utilities/Components/icon';
Copy link

Choose a reason for hiding this comment

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

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing
Use default import syntax to import 'I' import/no-named-default

import {Paragraph} from '../../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/typography';
import {Loading} from '../../../ignitus-Shared/ignitus-Utilities/Components/loader';
import {Normal} from '../../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/fonts';
Copy link

Choose a reason for hiding this comment

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

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing

font-weight: ${props =>
props.fontStyle === 'paragraph' ? F.Medium : F.Bold};
font-size: ${props => (props.fontStyle === 'paragraph' ? F.SM : F.MD)};
color: ${props =>
Copy link

Choose a reason for hiding this comment

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

Arrow function used ambiguously with a conditional expression no-confusing-arrow


export const Title = styled(Heading5)<TitleProps>`
padding: 0.5rem 2rem;
font-weight: ${props =>
Copy link

Choose a reason for hiding this comment

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

Arrow function used ambiguously with a conditional expression no-confusing-arrow

margin: 1.5rem 0;
`;

export const Title = styled(Heading5)<TitleProps>`
Copy link

Choose a reason for hiding this comment

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

Infix operators must be spaced space-infix-ops
Unexpected mix of '<' and '>' no-mixed-operators

Heading6,
} from '../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/typography';
import {TitleProps} from './types';
import {default as I} from '../../ignitus-Shared/ignitus-Utilities/Components/icon';
Copy link

Choose a reason for hiding this comment

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

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing
Use default import syntax to import 'I' import/no-named-default

Paragraph,
Heading6,
} from '../../ignitus-Shared/ignitus-DesignSystem/ignitus-Atoms/typography';
import {TitleProps} from './types';
Copy link

Choose a reason for hiding this comment

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

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing

</P.ParentContainer>
);

const Heading = ({title, icon, fontStyle}: HeadingProps) => (
Copy link

Choose a reason for hiding this comment

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

Parsing error: Unexpected token :

@@ -0,0 +1 @@
export {default} from './starCircleIcon';
Copy link

Choose a reason for hiding this comment

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

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing
Do not alias default as default. Just export default itself instead import/no-default-export

@@ -0,0 +1 @@
export {default} from './contributionIcon';
Copy link

Choose a reason for hiding this comment

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

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing
Do not alias default as default. Just export default itself instead import/no-default-export

Copy link
Member

@divyanshu-rawat divyanshu-rawat left a comment

Choose a reason for hiding this comment

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

LGTM :)

@divyanshu-rawat divyanshu-rawat merged commit 14efd4d into Ignitus:develop Apr 18, 2020
@divyanshu-rawat divyanshu-rawat mentioned this pull request Apr 18, 2020
@meghanacosmos meghanacosmos deleted the profileUI branch April 30, 2020 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard: Profile detail view
3 participants