Skip to content

Conversation

MichalVasut
Copy link
Contributor

No description provided.

@MichalVasut MichalVasut marked this pull request as draft September 30, 2022 15:22
@MichalVasut MichalVasut force-pushed the michal/simplify-views-registration branch 3 times, most recently from 77a5a59 to 47ff55b Compare October 4, 2022 19:10
@MichalVasut MichalVasut marked this pull request as ready for review October 4, 2022 23:02
@MichalVasut
Copy link
Contributor Author

It seems to be ready, live preview is here

https://apps.wikitree.com/apps/vasut2/views-registry/

Some docs rewrite is needed, but it works

@MichalVasut MichalVasut changed the title Simplify views registration - basic draft Simplify views registration Oct 5, 2022
@GeoffRiley
Copy link
Contributor

I don't think .prettierrc needs committing to the main git archive.

@MichalVasut
Copy link
Contributor Author

Sure, I can add it to .gitignore. The idea was to make some consensus about the best formatting and enforce it (possibly even via Github Action, that would format files automatically).
The reason is that I've noticed that every time somebody creates pull request, there are these formatting differences (like spaces in braces, singl vs double quotes, semicolons at the line end or not, line length - 80 / 120 characters,...).
At work, we are using similar tool for python (it's called Black) and it's comfortable and everybody is happy with it.

Copy link
Contributor

@GeoffRiley GeoffRiley left a comment

Choose a reason for hiding this comment

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

I don't think that .prettierrc should be committed, but beyond that There are a number of spelling and grammar errors that were corrected in 188e2b4 that have been missed during the creation of this. I've commented all the ones that I've spotted.

Hope that helps.

@GeoffRiley
Copy link
Contributor

Sure, I can add it to .gitignore. The idea was to make some consensus about the best formatting and enforce it (possibly even via Github Action, that would format files automatically). The reason is that I've noticed that every time somebody creates pull request, there are these formatting differences (like spaces in braces, singl vs double quotes, semicolons at the line end or not, line length - 80 / 120 characters,...). At work, we are using similar tool for python (it's called Black) and it's comfortable and everybody is happy with it.

I totally appreciate the idea behind your committing the file: but we don't all use the same tools: if there was something like black available I would be only too happy to suggest it. As it stands I attempt to follow whatever 'standard' the file is that I'm working on.

@bcaseyrls
Copy link
Contributor

Maybe we should have a codestyle.md documentation file with suggested style guides, and a copy of the .prettierrc file for those that can make use of it.

@GeoffRiley
Copy link
Contributor

Maybe we should have a codestyle.md documentation file with suggested style guides, and a copy of the .prettierrc file for those that can make use of it.

Sounds like a good idea to me. I like standards, but it's very difficult to get people to adhere to them without a formatter. 😁 Now everyone needs to prove me wrong. 😆

@MichalVasut
Copy link
Contributor Author

if there was something like black available I would be only too happy to suggest it.

I'm not really familiar with tools in javascript world. I'm mainly python coder, so I cannot serve, but from their web, it looks like it has quiet large support for different editors: https://prettier.io/docs/en/editors.html

What @bcaseyrls suggests is maybe the best solution.

@MichalVasut MichalVasut force-pushed the michal/simplify-views-registration branch 2 times, most recently from 329df38 to 0bce4fe Compare October 5, 2022 17:30
@MichalVasut
Copy link
Contributor Author

Hopefullly, this is final version

when there'll be general consensus about formating, I'll prepare codestyle.md document @bcaseyrls proposed

@GeoffRiley
Copy link
Contributor

I'm not really familiar with tools in javascript world. I'm mainly python coder, so I cannot serve, but from their web, it looks like it has quiet large support for different editors: https://prettier.io/docs/en/editors.html

I've just spent a happy hour or so looking at prettier and it does seem to be able to be integrated into a good variety of editors:

  • I've just brought it into PhpStorm to test it will an already 'opinionated' editor and it seems to integrate very well: no doubt the other Jetbrains products will equally be as simple to co-exist.
  • Insertion as a save filter for vi and emacs is a common kind of task and wouldn't cause a regular user any trouble to add.
  • I haven't tried putting it into Microsoft's Visual Studio Code, but it looks likely to be as simple as the Jetbrains systems.

Given the simplicity of integration, I have no hesitation in supporting this idea. I just wish I'd known about this earlier!

@MichalVasut
Copy link
Contributor Author

Hehe, I'm glad you like it 👍

I haven't tried putting it into Microsoft's Visual Studio Code, but it looks likely to be as simple as the Jetbrains systems.

I'm using it in VS Code and it integrates (via addon) very well, either as formatter on demmand or as autoformat during save - it can be configured in Settings of the VS Code or using that file.

@udjeni
Copy link
Contributor

udjeni commented Oct 5, 2022 via email

@MichalVasut
Copy link
Contributor Author

MichalVasut commented Oct 5, 2022

@udjeni, the file (config for formatting) is already in this branch as .prettierrc and is universal for prettier (editor independent).

Copy link
Contributor

@GeoffRiley GeoffRiley left a comment

Choose a reason for hiding this comment

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

All looks good to me.

@MichalVasut MichalVasut force-pushed the michal/simplify-views-registration branch from 0bce4fe to 4acbb23 Compare October 6, 2022 14:21
@MichalVasut
Copy link
Contributor Author

MichalVasut commented Oct 6, 2022

I've rebased this branch after yesterdays merge of Fan chart branch - can you, @Clarke-11007 , please check if I haven't messed it up - it looks suspicious and half working, but you've written something like that in your pull request.

If it's ok and there are no issues with this, can you, @bcaseyrls , please merge it. It's tiresome to solve merge conflicts everytime you merge something else. (I don't have rights to do so - definitelly safer to let only owner do the merges :-) )

@GeoffRiley already approved it yesterday, but the approval is probably gone after this rebase.

Thanks

PS: here is demo with Fan Chart included: https://apps.wikitree.com/apps/vasut2/views-registry/

@bcaseyrls bcaseyrls merged commit ea720cb into wikitree:main Oct 6, 2022
@bcaseyrls
Copy link
Contributor

@MichalVasut I've merged in your extensive work for View management.

This did not initially work for me when I ran it on a wikitree.com host. I ran into this problem with another view I'm working on, and the issue seems to be something not working with the PageSpeed module that's running for wikitree.com. That module combines and compresses JavaScript from pages. This breaks bare class definitions for some reason, so things like "View" were undefined when used later.

The workaround I've used is to just define the classes into the window:
window.ViewRegistry = class ViewRegistry { ... }

This is perhaps not correct, but fighting with PageSpeed (which otherwise has a number of benefits) was going to be a rabbit hole, and this at least did the trick. I don't get browser console errors and all of the views work for me.

The new main branch is up at https://apps.wikitree.com/apps/wikitree-dynamic-tree/.

@MichalVasut
Copy link
Contributor Author

@bcaseyrls thanks a lot. It would be better to mention those thinks before themerge and I would clean my mess myself, but I'm happy it's working now.

@bcaseyrls
Copy link
Contributor

Sorry if I stepped on your toes. I didn't notice the issue until I merged and tried running the new code on *.wikitree.com. I wanted it to work on apps, so I put in my workaround.

@Clarke-11007
Copy link
Contributor

HI there @MichalVasut - yes - it looks like the Fan Chart view is working as well as I've got it programmed to do so far. Thanks @bcaseyrls for incorporating that into the main branch (at least it looks like you've done that).

I like the new streamlined look to the top of the page - gives you what you need and doesn't waste any time or space doing so. Nicely done.

@bcaseyrls - is the window = class solution you've found and mentioned in a comment above here something that is specific to the ViewRegistry, or something that every view has to incorporate into their code?

@MichalVasut
Copy link
Contributor Author

No need for that, you did great to make it work. I don't really understand why it needs to be in global window object, but if it works, then ok, so be it.

What is the Pagespeed you've mentioned? Some js library? I briefly know about this one
https://pagespeed.web.dev/
But this seems to be only some reporting tool. 🤔

@bcaseyrls
Copy link
Contributor

@Clarke-11007 Yes, at least for now. It's a bit weird to deal with, but I've had to update timeline.js to also use "window.TimelineView = class TimelineView extends View". At some point I'll figure out why and maybe have a better solution, but for now I think it's more interesting to keep progress moving and this seems to do the trick.

@MichalVasut It is an Apache module (mod_pagespeed) - https://www.modpagespeed.com/. It runs in Apache and does stuff to content as its going out to the end user. In principle it shouldn't be messing with any code execution, but the way it bundles and compresses things apparently causes some sort of conflict, which I've resolved with the window object.

@MichalVasut
Copy link
Contributor Author

MichalVasut commented Oct 6, 2022

@bcaseyrls hmm, interesting, can you please specify the version of the Apache and the module itself that you are using? I'll try to run it in docker (I'll create minimal working environment) and do some experiments

@bcaseyrls
Copy link
Contributor

@MichalVasut Apache 2.4, mod_pagespeed-stable 1.13.35.2.

@MichalVasut MichalVasut deleted the michal/simplify-views-registration branch October 7, 2022 05:15
@udjeni
Copy link
Contributor

udjeni commented Oct 11, 2022 via email

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.

5 participants