-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
export to json option added #20
Conversation
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.
Looks good overall
few changes needed only
src/browser_actions/popup.js
Outdated
var exportLink = document.createElement("a"), | ||
exportBlob = new Blob([clipboardHistory], { type: "octet/stream" }), | ||
exportName = "clipboard-history.json", | ||
exportUrl = window.URL.createObjectURL(exportBlob); |
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.
use const here
@saifabusaleh I made the above changes. please check and let me know if anything remained to do or needs to fix. |
src/browser_actions/popup.js
Outdated
const exportLink = document.createElement("a"); | ||
var exportBlob = new Blob([clipboardHistory], { type: "octet/stream" }); | ||
const todaysDate = new Date().toLocaleDateString("en-GB"); | ||
const exportName = `clipboard-history-${todaysDate}.json`; |
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.
rename to export exportFileName
src/browser_actions/popup.js
Outdated
const clipboardHistory = JSON.stringify(clippingsList); | ||
const exportLink = document.createElement("a"); | ||
var exportBlob = new Blob([clipboardHistory], { type: "octet/stream" }); | ||
const todaysDate = new Date().toLocaleDateString("en-GB"); |
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.
rename to todayDate
src/browser_actions/popup.js
Outdated
@@ -173,3 +176,15 @@ const setDarkTheme = () => { | |||
htmlElement.classList.add("dark-theme"); | |||
themePath.setAttribute("d", sunSvgPath); | |||
} | |||
|
|||
const exportJson = () => { | |||
const clipboardHistory = JSON.stringify(clippingsList); |
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.
this doesn't take into account filtering
for example:
if user filtered the results and do export, all data will be exported instead of the filtered results
Can you take into consideration the filtering?
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.
@saifabusaleh is filtering feature is there already?
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.
@Sagargajare You can filter by searching text with the search input
@saifabusaleh I made it working with filtering as well. |
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.
Final comments
After finishing please rebase into single commit and I will merge
src/browser_actions/popup.js
Outdated
const exportJson = () => { | ||
let clipboardHistory = [] | ||
const filteredList = filterClippingsList(clippingsList, searchText); | ||
if (filteredList.length == 0) { |
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.
exported data should be always based on the search
if there is no data after the search, it should export empty json
@saifabusaleh I fixed the above issues. |
Thanks @Sagargajare Please squash all commits into one commit and I will merge to squash: git rebase -i master |
@saifabusaleh I rebased it. but it is saying "Everything up-to-date" |
@saifabusaleh I rebased it please check. |
Hi @Sagargajare That didn't work, you can see above the commits number is 30, it should be 1 Resources that can help you: https://stackoverflow.com/questions/14534397/squash-all-my-commits-into-one-for-github-pull-request https://medium.com/@mittalyashu/how-to-squash-commits-in-a-github-pull-request-97b36576eacb |
fbcb848
to
6f1891f
Compare
@saifabusaleh Thanks for suggesting these sources. Finally, it worked and was rebased to 1 commit. Please review it. Sorry for the delay. |
@Sagargajare Great, thanks |
export to json feature added export to json option added spacing and export icon fixed var name fixed export filtered data export filtered data export to json feature added export to json option added spacing and export icon fixed
Thanks @Sagargajare |
Thank You @saifabusaleh for guiding me. |
requested feature
#19 Add export to JSON option successfully added
After clicking the export icon in the left corner it downloads(export) the clipboard history to JSON format
It also supports light and dark themes.