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

Core Components Integration #131

Merged
merged 11 commits into from
Oct 6, 2021
Merged

Core Components Integration #131

merged 11 commits into from
Oct 6, 2021

Conversation

waralex
Copy link
Collaborator

@waralex waralex commented Oct 4, 2021

Integration of the core components into the Dash package.
The resources and meta-information of the components are placed in the artifact in the repository https://github.com/waralex/DashCoreResources. Before the release, I will move this repository to the Plotly organization.
Now the resources correspond to dash 2.0.0.
Also, before the release, it is necessary to release new versions of components without functions, but with a warning that using DashCoreComponents, etc. is depreciated.
I also changed the Dash version to 1.0.0

@github-actions github-actions bot added enhancement New feature or request tests labels Oct 4, 2021
src/Dash.jl Outdated Show resolved Hide resolved
end


const _reserwed_words = Set(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const _reserwed_words = Set(
const _reserved_words = Set(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in dbf86ff

)
end
end
function upload_to_resleases(repo_name, tag, tarball_path; attempts = 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function upload_to_resleases(repo_name, tag, tarball_path; attempts = 3)
function upload_to_releases(repo_name, tag, tarball_path; attempts = 3)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed with calls to this function in dbf86ff

test/runtests.jl Outdated
include("TestComponents.jl")
using .TestComponents
#include("TestComponents.jl")
#using .TestComponents
Copy link
Contributor

Choose a reason for hiding this comment

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

If these are no longer needed (looks like because dcc and html are included in this repo now?) can we delete these lines and the TestComponents.jlfile?

Similar question about dev.jl - you made changes to that file but it's commented out below, is it used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alexj dev.jl is a draft file that I use during the development of a new functionality if I need to call some function, etc. and see how it works. I.e. at the beginning of the development of a new functionality, I disable all tests except dev.jl, after the overall architecture emerges, I disable dev.jl and start writing full-fledged tests

Copy link
Collaborator Author

@waralex waralex Oct 6, 2021

Choose a reason for hiding this comment

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

can we delete these lines and the TestComponents.jlfile?

done in 87533d9

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it - that sounds useful, let's just put a comment at the top of dev.jl saying exactly ^^ so others know this - and so future reviewers don't flag it again. In general I have a very strong reaction "don't commit commented-out code, delete it!!!" but this serves a worthwhile purpose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let's just put a comment at the top of dev.jl

done in f59aae9

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Excellent work - excited to see this coming together!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants