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
Branches UI #446
Branches UI #446
Conversation
✅ Deploy Preview for bldrs-share ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
c6b06e6
to
fb0bd1a
Compare
@oo-bldrs when you have a chance, can you please take a look at the logic of the BranchControl component, and suggest the way to test it? |
"Versions" is a bit of an ambiguous term. In a Git repository, you can refer to a "version" through branches or tags. In the context of Git, a version would refer to a tag that generally starts with Maybe it's best to change the label to Git branch or another term that is less overloaded. |
src/Components/BranchesControl.jsx
Outdated
} | ||
branchesData.data.map((branch) => { | ||
modelPath.branch = branch.name | ||
const versionPath = `/share/v/gh/${modelPath.org}/${modelPath.repo}/${modelPath.branch}${modelPath.filepath}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between this invocation and the logic in <ShareRoutes>
, it makes sense to create a utility function for building a URL to a GitHub-hosted model -- this logic should no be duplicated in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oo-bldrs In the share routes, there is extractOrgPrefixedPath function, but I am not seeing where we are using ModelPath object to navigate.
Can you please point me to the function you have in mind that makes sense to include in the utility?
fdab4dd
to
98ed501
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, I'm seeing a lot of merge deltas.. does this need to be rebased or smth?
Also, do you have a link that shows the branches?
Found the Seestrasse one: Cool! |
98ed501
to
3cef2c8
Compare
getProperties: jest.fn(), | ||
} | ||
constructorMock.mockImplementation(() => impl) | ||
export {constructorMock as IfcViewerAPI} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pablo-mayrgundter not sure why this file is the part of the PR.
This reverts commit 05c9e91.
This reverts commit b7218fb.
This reverts commit a9e1b95.
-- Tests are missing