Separated data from presentation #110
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rands0n
reviewed
Jul 24, 2018
| } | ||
|
|
||
| function addProjectToProjectList(tag, project) { | ||
| var element = ''; |
There was a problem hiding this comment.
You think we can use a template generator here? Like Handlebars?
There was a problem hiding this comment.
But I do agree that is simple as create the template with element variable. Just thinking here...
|
Wow @kyleraymorgan, what a great change! Thanks for making these changes, I also think that is good to maintain because it's a simple JSON 😆. Can you fix the conflicts so I can proceed with the review? Also @azat-co what do you think about it? |
|
@rands0n Absolutely, a template generator would be a much better approach. But I wouldn't want to introduce dependencies without approval from the project owner. I'll resolve the conflicts tonight. |
|
Sorry, I'm not gonna have time to work on this anytime soon |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Edits made:
Reason for edits:
If you have the same HTML copy-and-pasted dozens of times, there will be errors made, and those errors will be hard to find. By separating the data (project information) out from the presentation (HTML), we've guaranteed that the HTML will be identical for every instance, and we've made the data easier to read/modify/add/delete because it's in a JSON object, not a buried in HTML. Any future HTML changes will only need to be made once, not dozens of times. And a project that belongs to multiple groups won't need to be duplicated, you just add another tag to the JSON object and it instantly appears in both groups.
Conclusion:
Feel free to reject/accept this as you wish. I think this will make the list easier to maintain going forward.