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.

Sign up for GitHub

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

Jump to bottom

Partially fixes #298 - make apps pluggable #299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Partially fixes #298 - make apps pluggable #299

wants to merge 1 commit into from

Conversation

djmgit
Copy link
Member

@djmgit djmgit commented Aug 11, 2017 •
edited
Loading

Partially fixes #298 - made apps pluggable

Partially fixes issue - #298, moved the following apps'
dependencies into their individual folders and updated
script and css references.

```
barchart
boilerplate
bubblecharts
collegeElections
compareTwitterProfiles
CountryTweetMap
ducphanduy
emojiheatmap
emojiheatmapper
fossasia-histogram
histogram
knowthediff
LQL
MultiLinePlotter

```

I have:

  • There is a corresponding issue for this pull request.
  • Mentioned the Issue number in the pull request commit message Fixes #<number> commit message
  • There is only strictly only one commit per issue.

For the reviewers

I have:

  • Reviewed this pull request by an authorized contributor.
  • The reviewer is assigned to the pull request.

vibhcool
vibhcool suggested changes Aug 12, 2017
View reviewed changes
Copy link
Member

@vibhcool vibhcool left a comment •
edited
Loading

Choose a reason for hiding this comment

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

May be I will be wrong, but IMO images in the apps shall also be moved to images directory in the respective apps.

@kavithaenair
Copy link
Member

@djmgit I think this PR will cause redundancy and space wastage.
As you can see the PR loklak/loklak_server#802, was to remove redundancy, but this PR may again cause the same issue.

@djmgit
Copy link
Member Author

djmgit commented Aug 15, 2017

@kavithaenair Its true that this will introduce redundancy, presently the apps have got their dependencies distributed in different folders (like js and css in root directory), so the apps are not standalone. The aim of this PR is to make the apps standalone so that any one can use an app independently (simply copy the app folder and use it somewhere else without caring about the dependencies). But that is only possible when the apps have all their dependencies with them (In the same folder) or else users will also have to manage the app dependencies as well that is also take care of the root js, css and images folder which contains dependencies of all apps.
If we use some dependency management tool like bower then too we will have to maintain bower components in each directory which will again give rise to the same issue - redundancy.

@Achint08 @hemantjadon @vibhcool @kavithaenair @singhpratyush @SKrPl what do you all suggest?

@vibhcool
Copy link
Member

vibhcool commented Aug 27, 2017 •
edited
Loading

IMHO Space shall not be much of the issue. If this PR keeps each app independent of each other and the website code and app code is kept separately, then this PR is fine.

@djmgit
Partially fixes loklak#298 - made apps pluggable
0e13eda 
Partially fixes issue - loklak#298, moved the following apps'
dependencies into their individual folders and updated
script and css references.

```
barchart
boilerplate
bubblecharts
collegeElections
compareTwitterProfiles
CountryTweetMap
ducphanduy
emojiheatmap
emojiheatmapper
fossasia-histogram
histogram
knowthediff
LQL
MultiLinePlotter

```
@djmgit djmgit changed the title [WIP] fixes #298 - make apps pluggable Partially fixes #298 - make apps pluggable Sep 2, 2017
@djmgit
Copy link
Member Author

djmgit commented Sep 2, 2017 •
edited
Loading

Test link: klakapps.surge.sh

@kavithaenair @vibhcool open for review.
I will be working on the remaining apps in subsequent PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vibhcool vibhcool vibhcool requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make apps plugable
3 participants
@djmgit @kavithaenair @vibhcool