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

Add json query support for index #53

Closed
wants to merge 1 commit into from
Closed

Add json query support for index #53

wants to merge 1 commit into from

Conversation

sdmcraft
Copy link

@sdmcraft sdmcraft commented Sep 21, 2023

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix #9

Seeking early feedback on this (currently DRAFT) PR for adding a generic querying mechanism for index.
Sunstar site uses article feeds heavily at many places wherein the feeds have filtering criteria (like related, featured, recent, category etc.) where this may be useful.

Test URLs:

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 21, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 21, 2023

Page Scores Audits Google
/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@bosschaert
Copy link

Looks useful to me. I wonder if there is an existing lightweight JS library that can maybe perform the query on the JSON?

Although I must admit I looked around briefly but couldn't find anything as lightweight as your json-query.js 😄

@sdmcraft
Copy link
Author

Looks useful to me. I wonder if there is an existing lightweight JS library that can maybe perform the query on the JSON?

Although I must admit I looked around briefly but couldn't find anything as lightweight as your json-query.js 😄

Yes, I tried searching around and couldn't find as specific as what I was looking for.

@@ -0,0 +1,55 @@
/* eslint-disable max-len */
export default function query(select, from, where, orderBy, rowCount) {

Choose a reason for hiding this comment

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

it would be good if we can include some test for this

/* eslint-disable max-len */
export default function query(select, from, where, orderBy, rowCount) {
// Validate input parameters
if (!Array.isArray(select) || !Array.isArray(from) || !Array.isArray(where) || where.length !== 3) {

Choose a reason for hiding this comment

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

can we replace this with !([select, from, where].every(Array.isArray) && where.length === 3)

@@ -332,6 +334,10 @@ export async function fetchIndex(indexFile, sheet, pageSize = 500) {
return newIndex;
}

export function queryIndex(index, select, where, orderBy, rowCount) {

Choose a reason for hiding this comment

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

why wrapper function is required here ?

from.sort((a, b) => {
const aValue = a[orderByField];
const bValue = b[orderByField];
if (sortOrder === 'asc') {
Copy link

@jindaliiita jindaliiita Sep 21, 2023

Choose a reason for hiding this comment

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

Conditions can be simplified by using Math.sign

Suggested change
if (sortOrder === 'asc') {
if (sortOrder === 'asc') {
Math.sign(aValue - bValue)
} else if (sortOrder === 'desc') {
Math.sign(bValue - aValue)
}

});

// Limit the number of rows based on the rowCount parameter
const selectedData = filteredData.slice(0, rowCount);

Choose a reason for hiding this comment

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

Can we add pagination support also in this ?

@sdmcraft sdmcraft added this to the Milestone-2 milestone Sep 22, 2023
@sdmcraft
Copy link
Author

Thanks for reviewing. I found a library (jslinq) which seems to do what we want, providing a richer api and is quite lightweight. Hence closing this PR and opening a new one #63 for it.

@sdmcraft sdmcraft closed this Sep 24, 2023
@sdmcraft sdmcraft removed this from the Milestone-2 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

News feed in cards block
3 participants