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

Feature/45 Right Sidebar Frontend implementation #57

Merged
merged 11 commits into from
Nov 8, 2022
Merged

Conversation

emekauja
Copy link

@emekauja emekauja commented Oct 20, 2022

Screenshot 2022-10-20 at 1 28 12 AM

@ddelpiano ddelpiano added the enhancement New feature or request label Oct 24, 2022
@ddelpiano ddelpiano linked an issue Oct 24, 2022 that may be closed by this pull request

const mockModel = require('../resources/model').mockModel;

const styles = () => ({
root: {
height: 'calc(100vh - 3.5rem)',
width: '100%',
margin: 'none',

Choose a reason for hiding this comment

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

@emekauja margin and padding values are not working, also i don't see the value add even id they were.

return (
<>
<Box paddingLeft="1.5rem" paddingY={1.5}>
<Typography component="body2" fontWeight={500}>

Choose a reason for hiding this comment

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

@emekauja body2 is a variant, not a component, hence no properties of body2 are reflecting

if (datasets?.length > 0) {
return (
<>
<Box paddingLeft="1.5rem" paddingY={1.5}>

Choose a reason for hiding this comment

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

@emekauja Incorrect padding should be an px passing instead of just left. Left would 2 not 1.5, so px={2}

@vidhya-metacell
Copy link

Hi @emekauja , i reviewed the UI of the sidebar, found the following issues:
Screen Shot 2022-10-26 at 2 04 06 PM

  • The width is not like figma, border and border color do not match to figma and there is no box shadow in figma

Screen Shot 2022-10-26 at 2 13 49 PM

  • The arrow icon is different in figma.

Screen Shot 2022-10-26 at 2 17 13 PM

- There is no space/padding on the left of the arrow icon in figma

Screen Shot 2022-10-26 at 2 30 02 PM

  • Once i expand this composition, i can never collapse it. always remains expanded.

Screen Shot 2022-10-26 at 2 32 59 PM

  • The child icon should be vertically aligned with the Composition text

Screen Shot 2022-10-26 at 2 37 56 PM

  • The arrow icon of the child B2 should be aligned with Projection A2 icon

Screen Shot 2022-10-26 at 2 44 50 PM

  • These two icons, do not have as much spacing between each other, please refer to figma again. I see extra padding left and margin-right adding this space. Overall spacing between these elements should be considered again using figma.

@vidhya-metacell
Copy link

vidhya-metacell commented Oct 26, 2022

@emekauja found the following issues with font properties

Screen Shot 2022-10-26 at 2 51 45 PM

  1. The overall letter spacing should be as in figma
  2. The height of the row is 36 not 42. Please keep an eye on the details.

Screen Shot 2022-10-26 at 2 53 04 PM

  • On hover the background color changes to white, it should remain the same.

Screen Shot 2022-10-26 at 2 56 29 PM

- The font weight of the composition heading should be 500, in figma is see 510, please use 500 instead of 600. - remove letter spacing its not there in figma

Screen Shot 2022-10-26 at 2 59 02 PM

  • use font weight same as composition heading for the labels, 500
  • fix the line height
  • remove letter spacing, keep it normal

Screen Shot 2022-10-26 at 3 00 51 PM

  • Label is missing this 2px margin

Screen Shot 2022-10-26 at 3 02 14 PM

  • Values for the line height, letter spacing, font size , font color, are not matching figma

Screen Shot 2022-10-26 at 3 05 21 PM

  • From the box, flex, flex direction, align-items can be removed, not adding anything. Text overflow ellipsis should be removed, will not work on box. Ellipses should be on p tag

Copy link
Member

@ddelpiano ddelpiano left a comment

Choose a reason for hiding this comment

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

There is another problem noticed testing the sidebar.

If I click on the first composition and then try to compress this, the compression does not work unless I play with another item of the sidebar (deadlock in the state?), please fix this.

scrnli_01_11_2022_10-12-55.mp4

Other than this there are the changes requested by Vidhya and then this can be approved.

Thanks!

return;
}

// if (nodes.length !== nodeIds.length && nodes[0] === nodeIds[0]) {
Copy link
Member

Choose a reason for hiding this comment

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

@emekauja please remove commented code if not used

@emekauja emekauja requested a review from ddelpiano November 7, 2022 22:30
@ddelpiano ddelpiano merged commit fd610d9 into metacell Nov 8, 2022
Salam-Dalloul pushed a commit that referenced this pull request Jun 5, 2023
Salam-Dalloul pushed a commit that referenced this pull request Jun 9, 2023
Salam-Dalloul pushed a commit that referenced this pull request Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composition Sidebar UI
3 participants