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

Add Issues/Notes #235

Merged
merged 45 commits into from Jul 8, 2022
Merged

Add Issues/Notes #235

merged 45 commits into from Jul 8, 2022

Conversation

OlegMoshkovich
Copy link
Member

No description provided.

@netlify
Copy link

netlify bot commented Jun 21, 2022

Deploy Preview for bldrs-share ready!

Name Link
🔨 Latest commit c29d49d
🔍 Latest deploy log https://app.netlify.com/sites/bldrs-share/deploys/62c797b17c853e0008fd48a0
😎 Deploy Preview https://deploy-preview-235--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.

@pablo-mayrgundter
Copy link
Member

Really nice to see this :)

Per conversation:

  1. Looks like we have a regression at some point in the last PRs.. loading a new model doesn't reset state of either ItemProps or Comments. This would be good to add a unit test for; generally a good strategy that any bug that makes it into production should have tests associated with the fix.

  2. Mobile drawer ought to slide up from bottom.

@OlegMoshkovich
Copy link
Member Author

Morning! Addressed the store reset and the mobile drawer issues.

src/Components/IssuesControl.jsx Show resolved Hide resolved
src/Components/IssuesControl.jsx Outdated Show resolved Hide resolved
src/Components/CameraControl.jsx Outdated Show resolved Hide resolved
src/Components/CameraControl.jsx Outdated Show resolved Hide resolved
src/Components/CameraControl.jsx Outdated Show resolved Hide resolved
src/Components/IssueCard.jsx Outdated Show resolved Hide resolved
src/Components/IssueCard.jsx Outdated Show resolved Hide resolved
src/Components/OperationsGroup.jsx Show resolved Hide resolved
src/Components/OperationsGroup.jsx Outdated Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
@OlegMoshkovich
Copy link
Member Author

@pablo-mayrgundter PTAL

add ifc slice to the store

add ifc slice state properties to cadview

add select IFC element test

add viewer test

add viewer test

new build

add ifc slice

clean up

clean up

add side drawer

new build

Add sideDrawer to cadView

clean up

add properties panel to the side drawer

add hightlight color to the theme

delete item panel drawer

add item properties test

clean up

clean up

clean up

fix mobile drawer

clean up

styles

refactor title out of the side drawer panel

resolve comments

add issue card + rewire issue control

add camera control

rewire operations group

rewire operations group

add Issue Slice to the store and connect it to operations group

add issue panel

style side Drawer

testing drawer on the top

show small image on mobile

new build

drawer style on mobile

drawer styles

style

mobile drawer

reset element state on model

style

style

fix camera

clean up

add camera controls

new build

addressed comments

new build

resolve comments

fixes
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.

fixed spacing with eslint rules
@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.

Octokit mock for local testing needs to be restored. Besides that just some formatting stuff.. please use spaces never for now in the new eslintrc rule and you should be able to fix automatically with:

yarn eslint --fix src/.../yourfile

src/Components/IssueCard.jsx Outdated Show resolved Hide resolved
src/Components/IssueCard.jsx Show resolved Hide resolved
src/Components/IssueCard.test.jsx Outdated Show resolved Hide resolved
src/Components/IssueCard.test.jsx Outdated Show resolved Hide resolved
src/Components/IssueControl.test.jsx Outdated Show resolved Hide resolved
src/Components/SideDrawer.jsx Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
src/utils/GitHub.js Show resolved Hide resolved
src/utils/GitHub.js Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
@OlegMoshkovich
Copy link
Member Author

@pablo-mayrgundter addressed everything - besides 'spaces never' - I think.

@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.

Let's talk about the last unit test to exercise the code getting the hash from useLocation through to IssueCard

src/utils/GitHub.js Outdated Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
src/utils/GitHub.js Outdated Show resolved Hide resolved
src/Components/IssuesControl.jsx Outdated Show resolved Hide resolved
src/Components/OperationsGroup.jsx Outdated Show resolved Hide resolved
src/Containers/CadView.jsx Outdated Show resolved Hide resolved
src/Components/OperationsGroup.jsx Outdated Show resolved Hide resolved
src/Components/SideDrawer.test.jsx Show resolved Hide resolved
src/Components/SideDrawer.test.jsx Show resolved Hide resolved
src/Components/SideDrawer.jsx Show resolved Hide resolved
/** The prefix to use for issue id in the URL hash. */
export const ISSUE_PREFIX = 'i'
useEffect(() => {
const fetchIssues = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please add:

// TODO(pablo): refactor issues fetch logic into Issues.js

@pablo-mayrgundter
Copy link
Member

Please add the one TODO comment I noted, then all good!

@pablo-mayrgundter
Copy link
Member

Actually, I want to base some changes off of this so will merge now and add the todo.

@pablo-mayrgundter pablo-mayrgundter merged commit 3e6084c into bldrs-ai:main Jul 8, 2022
pablo-mayrgundter added a commit that referenced this pull request Jul 8, 2022
@OlegMoshkovich OlegMoshkovich deleted the Notes_2 branch February 14, 2024 16:15
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