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

fix: undefined glyphType and nested lists #5

Merged
merged 6 commits into from Mar 7, 2023

Conversation

Yankie
Copy link
Contributor

@Yankie Yankie commented Mar 2, 2023

It seems like GoogleDocs API does not set glyphType for unordered lists

It seems like GoogleDocs API does not set `glyphType` for unordered lists
@google-cla
Copy link

google-cla bot commented Mar 2, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jpoehnelt jpoehnelt changed the title Fix for UL fix: undefined glyphType Mar 2, 2023
@jpoehnelt
Copy link
Member

Probably need to regenerate some of the snapshots which were probably incorrect. I think npm test -- --update should do that.

@Yankie
Copy link
Contributor Author

Yankie commented Mar 2, 2023

Probably need to regenerate some of the snapshots which were probably incorrect. I think npm test -- --update should do that.

Moreover, it seems your reduce func in transform at src/hast/index.ts:93 joins all list items in one list later at lines 123-144...
But there should be 2 lists - ul with 1 nested ul and ol with 1 nested ol... May be you could distinguish them by bullet value

I'm not sure yet how and where it should be treated - at hast level or later at html, but i've just started :)

@Yankie
Copy link
Contributor Author

Yankie commented Mar 3, 2023

Since I'm new to TS, could you please help me with this? Thanx in advance!

...
// get last child Element from element's children
const getLastElementChild = (el: Element): Element => {
  return el.children.filter((e) => {e.type === "element"}).at(-1);
};
...

I've got error about Type 'ElementContent' is not assignable to type 'Element':

src/hast/index.ts:106:3 - error TS2322: Type 'ElementContent' is not assignable to type 'Element'.
  Type 'Comment' is missing the following properties from type 'Element': tagName, children

106   return el.children.filter((e) => {e.type === "element"}).slice(-1)[0];

@Yankie Yankie requested a review from jpoehnelt March 3, 2023 19:41
@Yankie
Copy link
Contributor Author

Yankie commented Mar 3, 2023

Since I'm new to TS, could you please help me with this? Thanx in advance!

...
// get last child Element from element's children
const getLastElementChild = (el: Element): Element => {
  return el.children.filter((e) => {e.type === "element"}).at(-1);
};
...

I've got error about Type 'ElementContent' is not assignable to type 'Element':

src/hast/index.ts:106:3 - error TS2322: Type 'ElementContent' is not assignable to type 'Element'.
  Type 'Comment' is missing the following properties from type 'Element': tagName, children

106   return el.children.filter((e) => {e.type === "element"}).slice(-1)[0];

Nevermind, I've figured it out!

// get last child Element from element's children
const getLastElementChild = (el: Element): Element => {
  return el.children.filter((e) => {e.type === "element"}).at(-1) as Element;
};

@Yankie Yankie marked this pull request as draft March 3, 2023 22:35
@Yankie Yankie marked this pull request as ready for review March 3, 2023 22:38
@Yankie
Copy link
Contributor Author

Yankie commented Mar 5, 2023

Tomorrow, will fix case when list levels of current and prev elements diff by 2 and more

@jpoehnelt
Copy link
Member

Thanks for the work here. Tests pass now but sounds like there is another test case from you last comment?

@Yankie
Copy link
Contributor Author

Yankie commented Mar 6, 2023

@jpoehnelt i'm done!

@Yankie
Copy link
Contributor Author

Yankie commented Mar 6, 2023

@jpoehnelt jpoehnelt changed the title fix: undefined glyphType fix: undefined glyphType and nested lists Mar 7, 2023
@jpoehnelt jpoehnelt merged commit 878d0bd into googleworkspace:main Mar 7, 2023
github-actions bot pushed a commit that referenced this pull request Mar 7, 2023
* Fix for UL

It seems like GoogleDocs API does not set `glyphType` for unordered lists

* Fixed nested lists

* Fixed case where `el` list level is larger by more than 2 from `last`'s level 878d0bd
googleworkspace-bot pushed a commit that referenced this pull request Mar 7, 2023
## [1.0.2](v1.0.1...v1.0.2) (2023-03-07)

### Bug Fixes

* undefined glyphType and nested lists ([#5](#5)) ([878d0bd](878d0bd))

### Build System

* use correct registry ([49ffadd](49ffadd))
* use current node version in release ([20d1a05](20d1a05))
@jpoehnelt
Copy link
Member

@Yankie do you mind sharing your use case for this library. You can email me at jpoehnelt@google.com. Thanks! Looks like 1.0.2 is available on NPM now.

@Yankie
Copy link
Contributor Author

Yankie commented Mar 7, 2023

Sure! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants