blueprintjs removal #4006

maxphilippov · 2024-07-04T07:24:37Z

This is an attempt to remove BlueprintJS from the project. This is pretty huge in terms of what it changes so I might have missed something, but I believed that the things came out pretty simple and manageable. What I saw during this cleanup is that we've redefined a lot of BP styling etc. making it almost our own implementation.

BlueprintJS is gone from package.json so you have to reinstall node_modules to check this properly.

Things missing:

I didn't handle the Escape key bind to close new Dialog. I saw other discussions (esc-key in group create & -edit close the whole stack, not the last dialog #2268, Settings should be closable in sub-pages with escape key #3868) regarding how we should handle a stack of open windows and close only the latest one.
I didn't remove bp4- class overrides (almost all of them done in either dev_rocket or dev_minimal themes), so this is definitely an open discussion (should we make up new names, add these classes to new elements or we can come up with something else). If you worked on these themes, please hit me up.

It also opens up an easier way to update React since we don't have to manage BlueprintJS too.

WofWca · 2024-07-04T07:59:10Z

About dialogs: not sure if you're aware, but now HTML has a native <dialog> which handles all these dismiss and stack things pretty well.

maxphilippov · 2024-07-04T08:51:50Z

src/renderer/components/Login-Styles.tsx

    )
  }
 )

 export const DeltaProgressBar = function (
  props: React.PropsWithChildren<{
    progress: number
-    intent?: Intent
+    intent?: string


I forgot to actually implement that, commit incoming

farooqkz · 2024-07-04T11:34:15Z

I can't do a review on the PR today(can't concentrate due to bad sleep pattern). But my opinion is mixed. You are doing BP removal which is desired but I am not in favor of big PRs as they need extensive testing and review. IMO, in such a bug codebase, a big PR might do more bad than good unless the review and testing is really extensive. Nevertheless, As I said removal of BP is very much wanted as a major upgrade of React and many other changes are softly dependent on it.

farooqkz · 2024-07-04T12:39:02Z

BTW, It's good that you haven't touched the Gallery component as there is another PR for it.

maxphilippov · 2024-07-04T14:08:08Z

I can't do a review on the PR today(can't concentrate due to bad sleep pattern). But my opinion is mixed. You are doing BP removal which is desired but I am not in favor of big PRs as they need extensive testing and review. IMO, in such a bug codebase, a big PR might do more bad than good unless the review and testing is really extensive. Nevertheless, As I said removal of BP is very much wanted as a major upgrade of React and many other changes are softly dependent on it.

Yeah, I don't think it's a good idea to dive into code straight away, I'd rather have people checking all the interfaces they use daily and look at what broke in terms of layout.

I know it looks like a huge change but from what I saw we used BP just for markup, the only thing that has some kind of interactive behavior is Dialog (we lost a lot of interactive things after replacing map with a new one opening in a popup). So it should be pretty obvious if something broke by just the look of it. I'm trying to do a screen by screen comparison, but that still requires some time.

Simon-Laux

Bugs from testing:

clicking on any page in settings just closes the dialog and does not work.
new group in create chat dialog has the same issue
clicking on contacts in group edit/view dialog also closes the dialog and does not do what it is supposed to do
Icons in gallery look different

should not be blue

tabs are invisible in dark theme

Dialog footer items in add as second device are not aligned at the start and end.

tabs in scan qr code dialog look different

Simon-Laux · 2024-07-04T20:54:38Z

scss/_icons.scss

-    & > svg {
-      fill: #bf0000;
-    }
-  }
 }


was the removal of the hover effect intentional?

Not intentional, but I moved it to <Icon coloring in latest commit, thanks for catching this

Simon-Laux · 2024-07-04T21:25:18Z

I think we should forget about using Overlay for dialog and try to use the <dialog> element that the browser already provides, it even already has a ::backdrop css selector.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

I guess we should first try to use them with the open attribute without showModal, that we don't need to rewrite the dialog manager, but if that doesn't work we might need to rewrite it. also I wonder if the html5 dialog element supports multiple dialogs on top of each other.

If the <dialog> element doesn't work then we need to adjust the overlay component:

make sure the onClick handler is on the backdrop and renamed to onBackdropClick
make sure the overlay content is centered in a way that it does not block the backdrop from being clicked.

(thats the bug that prevents me from using the settings dialog)

farooqkz · 2024-07-11T12:59:42Z

@maxphilippov after you addressed the bugs Simon has found, ping me to do a review by using.

maxphilippov · 2024-07-18T12:13:55Z

I think we should forget about using Overlay for dialog and try to use the <dialog> element that the browser already provides, it even already has a ::backdrop css selector. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog

I guess we should first try to use them with the open attribute without showModal, that we don't need to rewrite the dialog manager, but if that doesn't work we might need to rewrite it. also I wonder if the html5 dialog element supports multiple dialogs on top of each other.

If the <dialog> element doesn't work then we need to adjust the overlay component:

make sure the onClick handler is on the backdrop and renamed to onBackdropClick

make sure the overlay content is centered in a way that it does not block the backdrop from being clicked.

(thats the bug that prevents me from using the settings dialog)
also replying to @WofWca

So I've looked into that and I personally find the very rough and actually bad to use.

I pushed a reference implementation I used to figure things out.

My biggest problem with it is the desing which is kinda orthogonal to React idea of 'you specify element visibility by its presence in the DOM'.

This one is stateful and that makes it do excessive work which we don't even use.

Our current system (and I would argue it's a sane one) has a simple principle:

there's no 'closed' state for dialog, if it's in DOM it's open

There's only one way to open through markup and it's the only property 'open':
that gives us neither backdrop nor 'close on escape'
So we have to do useEffect and call showModal() every time, cause we don't have a 'closed' state.

Do we need a 'closed' state? I don't think so. That would require either to have all dialogs present in DOM or to have a manager to cleanup unused dialogs. I don't see any of these being useful.

We also need to have CSS-display: flex on the dialog, which conflicts with how manages display.

We already have a stack of opened dialogs in Dialog Context and it's implemented as natural as possible with array.
So storing it's own order is again excessive work.

What I'm doing in reference impl. is using it's onClose callback to send a message to our DialogContext it's time to delete this one right after it get's hidden.

I'm not an expert on accessibility but it seems like it doesn't even set aria-modal="true"

maxphilippov · 2024-07-18T12:15:18Z

Bugs from testing:

Most of the dialog things should be fixed, but there's also a clear discrepancy between our styles. I didn't have a problem with flex dialog footer/header. But I had issues with basic text color in dialog. Can you re-check those please?

I didn't want to touch Gallery since @farooqkz is working on this, maybe we can handle it the same you @Simon-Laux did the follow-up PR to this one?

nicodh · 2024-07-18T14:52:23Z

@maxphilippov could you please go through the comments and answer Simons in detail (unresolved conversations)
For example which of the reported bugs are fixed.
Would be helpful for reviews...

Simon-Laux · 2024-07-18T15:16:50Z

can you check the todo boxes in my comment for the issues that you have addressed?

farooqkz · 2024-07-18T18:26:33Z

@maxphilippov I think it's a good idea that I finish my Gallery BP removal and that you base your PR on it? Because one of the bugs @Simon-Laux has found directly is part of Gallery. Plus I'm doing some refactors there and introducing "Tabs" component which you could use.

Simon-Laux · 2024-07-18T18:40:16Z

max pr is bigger so I guess the other way around would make more sense? (rebase #3977 to this pr and change it so it should merge into maxph/blueprintjs_removal_part2)

- replace these components from blueprintjs with our own implementation * Overlay, Navbar, Dialog, Switch, InputGroup, FormGroup, Intent - new icons: eye-open, eye-off - rm blueprint import from scss - rm blueprint from electron-builder ignorelist - reference implementation using <dialog>

maxphilippov requested review from nicodh, adzialocha, r10s, farooqkz, Simon-Laux and adbenitez July 4, 2024 07:24

maxphilippov marked this pull request as draft July 4, 2024 07:30

maxphilippov mentioned this pull request Jul 4, 2024

blueprintjs removal part1 #3857

Closed

maxphilippov force-pushed the maxph/blueprintjs_removal_part2 branch from d768770 to f3f3907 Compare July 4, 2024 07:35

maxphilippov commented Jul 4, 2024

View reviewed changes

Simon-Laux requested changes Jul 4, 2024

View reviewed changes

Simon-Laux reviewed Jul 4, 2024

View reviewed changes

Simon-Laux added refactor This PR or issue is about refactoring technical debt labels Jul 5, 2024

maxphilippov force-pushed the maxph/blueprintjs_removal_part2 branch 2 times, most recently from 1263d2b to f9bf66d Compare July 18, 2024 12:25

This was referenced Jul 18, 2024

upgrade react to v18 and react pinch pan to v3 #3958

Draft

Remove BP dep from Gallery #3977

Draft

maxphilippov changed the title ~~Maxph/blueprintjs removal~~ blueprintjs removal Jul 24, 2024

maxphilippov force-pushed the maxph/blueprintjs_removal_part2 branch from f9bf66d to 0c77fad Compare September 3, 2024 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blueprintjs removal #4006

blueprintjs removal #4006

maxphilippov commented Jul 4, 2024

WofWca commented Jul 4, 2024

maxphilippov Jul 4, 2024

farooqkz commented Jul 4, 2024

farooqkz commented Jul 4, 2024

maxphilippov commented Jul 4, 2024

Simon-Laux left a comment •

edited by maxphilippov

Loading

Simon-Laux Jul 4, 2024

maxphilippov Jul 18, 2024 •

edited

Loading

Simon-Laux commented Jul 4, 2024 •

edited

Loading

farooqkz commented Jul 11, 2024

maxphilippov commented Jul 18, 2024

maxphilippov commented Jul 18, 2024 •

edited

Loading

nicodh commented Jul 18, 2024

Simon-Laux commented Jul 18, 2024

farooqkz commented Jul 18, 2024

Simon-Laux commented Jul 18, 2024 •

edited

Loading

blueprintjs removal #4006

Are you sure you want to change the base?

blueprintjs removal #4006

Conversation

maxphilippov commented Jul 4, 2024

WofWca commented Jul 4, 2024

maxphilippov Jul 4, 2024

Choose a reason for hiding this comment

farooqkz commented Jul 4, 2024

farooqkz commented Jul 4, 2024

maxphilippov commented Jul 4, 2024

Simon-Laux left a comment • edited by maxphilippov Loading

Choose a reason for hiding this comment

Simon-Laux Jul 4, 2024

Choose a reason for hiding this comment

maxphilippov Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Simon-Laux commented Jul 4, 2024 • edited Loading

farooqkz commented Jul 11, 2024

maxphilippov commented Jul 18, 2024

maxphilippov commented Jul 18, 2024 • edited Loading

nicodh commented Jul 18, 2024

Simon-Laux commented Jul 18, 2024

farooqkz commented Jul 18, 2024

Simon-Laux commented Jul 18, 2024 • edited Loading

Simon-Laux left a comment •

edited by maxphilippov

Loading

maxphilippov Jul 18, 2024 •

edited

Loading

Simon-Laux commented Jul 4, 2024 •

edited

Loading

maxphilippov commented Jul 18, 2024 •

edited

Loading

Simon-Laux commented Jul 18, 2024 •

edited

Loading