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

Redesign #395

Merged
merged 45 commits into from Sep 23, 2022
Merged

Redesign #395

merged 45 commits into from Sep 23, 2022

Conversation

OlegMoshkovich
Copy link
Member

@OlegMoshkovich OlegMoshkovich commented Sep 16, 2022

PR includes redesign(restyle) of the following components:

  • Buttons
  • Dialog
  • SnackBar
  • Search
  • Open Dialog
  • Share Dialog
  • About Dialog
  • NavTree
  • Item Properties
  • Item Properties refactor
  • SideDrawer
  • Replacement of Icons
  • Theme adjustments

@netlify
Copy link

netlify bot commented Sep 16, 2022

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit 3f36305
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/632dc76f826b590009e95cc4
😎 Deploy Preview https://deploy-preview-395--bldrs-share.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@OlegMoshkovich OlegMoshkovich marked this pull request as draft September 16, 2022 14:07
@OlegMoshkovich OlegMoshkovich marked this pull request as ready for review September 17, 2022 12:05
Copy link
Member Author

@OlegMoshkovich OlegMoshkovich left a comment

Choose a reason for hiding this comment

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

@pablo-mayrgundter please take a look.

src/Components/AboutControl.jsx Outdated Show resolved Hide resolved
src/Components/Buttons.test.jsx Outdated Show resolved Hide resolved
result.current.setIssues(MOCK_ISSUES)
})
await act(() => {
result.current.setComments(MOCK_COMMENTS)
})
Copy link
Member Author

Choose a reason for hiding this comment

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

these cases are failing, but comments are loaded.
not totally sure whats going on here.
commenting out for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how it happened but the failure of these test cases is related to the bug that made its way into Production.
At the moment when the issue card is clicked, the view changes to the individual issue but the comments do not display.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

the bug is filed here #400

src/Components/ItemProperties.jsx Show resolved Hide resolved
@OlegMoshkovich
Copy link
Member Author

@pablo-mayrgundter PTAL

Copy link
Member

@pablo-mayrgundter pablo-mayrgundter left a comment

Choose a reason for hiding this comment

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

Sitting with Markus.. some style points:

  • Font weights should go from heavy to light as you narrow scope. So for instance, in properties, the attribute names should have a heavier weight than the values.
  • Same principle for SideDrawer: separator between scene and drawer content should be heavier than between rows in properties

Code style: Lots of imports changed; pls proofread them for sorting. I'm seeing them out-of-order in a few files. See style guide on imports here: https://github.com/bldrs-ai/Share/wiki/Dev:-Style#imports

@@ -1,15 +1,14 @@
import React, {useState, useEffect} from 'react'
Copy link
Member

Choose a reason for hiding this comment

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

Pls sort imports.. looks like they got mixed up over time

Copy link
Member Author

Choose a reason for hiding this comment

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

I just resorted them - took a long time actually - following the comment you made yesterday.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, will check.. thought I saw some out of order

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants