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 for hidden column issue fixes #981 #985

Merged
merged 5 commits into from
Jan 29, 2024
Merged

fix for hidden column issue fixes #981 #985

merged 5 commits into from
Jan 29, 2024

Conversation

6pac
Copy link
Owner

@6pac 6pac commented Jan 21, 2024

Testing fix - no action needed yet

@6pac
Copy link
Owner Author

6pac commented Jan 22, 2024

@ghiscoding sorry to ask again, but I have updated the ts - what is the easiest way to get it compiled to the dist folder for testing? Either on GiHub so I can hit the example URL, or locally.

Also, I think this will fix all of the problems. You might want to test it.

@ghiscoding
Copy link
Collaborator

ghiscoding commented Jan 22, 2024

You'll have to update your PR though because you overwrote some of my code since your branch is slightly out of sync with master.

For the build, you can run either Dev mode or run a Prod Build with TypeScript Types

npm run dev           # without TS Types
npm run build:prod    # also builds TS Types

just running in Dev mode is typically enough to test, just don't include any of the files from the dist/.. folder in the PR, the dist folder should only be updated when releasing a new version.

I'll test it tonight after work, I thought it would you a few days so I already released a version during the weekend, we can push a patch version later though

@6pac
Copy link
Owner Author

6pac commented Jan 22, 2024

Thanks. Yeah I noticed the leakage. A bit weird since I ran a pull, created the branch and the PR all within about 10 minutes.

@ghiscoding
Copy link
Collaborator

@6pac I just tried it and it's still not correct as far as I can see, the "Finish" column header still shows up when hiding it
brave_xApCwpshYD

@6pac
Copy link
Owner Author

6pac commented Jan 26, 2024

@ghiscoding it's a bit embarrassing, but I really can't work the TS tooling for local testing.

I tested in js so I know the patch works. It's just getting the ts to flow through to dist/browser that is causing problems. I tried the dev and prod builds (had to update nodejs to get rimraf to work), and it's changing the dist files but not updating them with the ts changes.

For example, the grid init is failing with

Uncaught TypeError: grid.getPubSubService is not a function

and getPubSubService is in the ts file but not any js.

I notice that there is a .js file in the src folder for each .ts file. It this .js file used or is it just an artefact of tsc?
I'm not running tsc manually, I'm expecting the dev or prod build to do that.

What am I missing?

@ghiscoding
Copy link
Collaborator

ghiscoding commented Jan 26, 2024

that's weird, rimraf is supposed to delete the dist folder you could try to do that manually before starting because technically the process is to simply run npm run dev and use the example. However very important, when you start dev it should open a new browser window and you need to run the examples from there. You cannot test the example like before, so you cannot double click an example from file explorer to run them, you will not get latest code doing that way. You really have to run it from npm run dev, so you'll get a link similar to this for basic example http://localhost:8080/examples/example1-simple.html

Also make sure that you ran npm install, I updated the dependencies a few times (almost every time I do a release).

I notice that there is a .js file in the src folder for each .ts file. It this .js file used or is it just an artefact of tsc?
I'm not running tsc manually, I'm expecting the dev or prod build to do that.

so inside the dist/browser folder that is normal and the expected behavior to get a transpiled JS file for each TS file from src, it is that way so that long time user of SlickGrid loading through <script> will have similar approach as before and load any plugins they way by loading whichever plugin JS file.

On the other hand, we have the CJS and ESM folders, those are single output file because new tools like RollupJS/Vite/WebPack will take care of the tree shaking, that process is executed when the user does a Prod build, it will only keep only the code you imported and remove anything else that is not associated to the import. For example if I do import { SlickColumnPicker, SlickGrid } from 'slickgrid', then the Prod build will only include code related to these 2, anything else will be removed from the final Prod build JS file.

In conclusion, the dist/browser cannot do that process because when you load through <script>, you are loading the entire content of that file, there is no tree shaking process involved which is why we need to keep separate JS files so that user can choose which JS file to load without loading everything else (you will never have a user that loads everything or at least very rarely, so for most users you want to keep their download size small)

you should get something like below in your folders

dist folder

image

src folder

image

@ghiscoding
Copy link
Collaborator

So I tried it again and I still see the same problem as before

@6pac
Copy link
Owner Author

6pac commented Jan 27, 2024

OK, yes have got it working, I don't know what was going on before. There was some locking of npm packages that I had to reboot to get rid of, I think maybe npm was having a moment.
I also had .js files in my src folder, that must have been from when I was first checking out tsc. that was confusing and I have deleted them all.

Anyway, thanks for the advice and try again now. There was a single line of code missing.

I am aware of the purpose of the cjs and esm folders, although I don't meddle with them. Again thanks for the rundown.

@ghiscoding
Copy link
Collaborator

ah yeah that last missing line does seem to fix the issue for good :)

if (c && c.resizable && !c.hidden) {
for (j = i + 1; j < vc.length; j++) {
c = vc[j];
if (c && c.resizable) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could use optional chaining here ?. you can see more info on MDN optional chaining

c = this.columns[j];
if (c && c.resizable && !c.hidden) {
c = vc[j];
if (c && c.resizable) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could use optional chaining here too ?.

@ghiscoding ghiscoding merged commit 02d0fd2 into master Jan 29, 2024
2 checks passed
@ghiscoding ghiscoding deleted the hidden-col branch January 29, 2024 14:23
@ghiscoding
Copy link
Collaborator

@6pac you can probably give a try with the new structure and push a new release, just run npm run release. The process is the same as before, the only difference is that I added a new OTB (One Time Password) question, just type ENTER when that question comes since I think you're not using OTP yourself.

I think we're pretty much done with fixes and everything, so pushing a new release would be a good time I think. Cheers

@6pac
Copy link
Owner Author

6pac commented Feb 4, 2024

@ghiscoding I'm happy for you to push releases (I think you've done the last few?). I do it so infrequently that it's likely I'll have tooling problems or forget to do something. Can do it if you really want, but I think it'd be better if I just did them if there was a need for me to do a number in a row.

@ghiscoding
Copy link
Collaborator

sure I can do it, I thought you maybe wanted to practice the releases 😋

@6pac
Copy link
Owner Author

6pac commented Feb 5, 2024

Nope ;-)

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.

None yet

2 participants