refactor: overhaul of the ComponentDemo, split into multiple files, i…#968
Conversation
…mproved file passing, fixed bugs and logical errors, and updated markdown files for new props usage
MatthewHawkins
requested review from
EHandtkeBasis,
bbrennanbasis,
gosteenBASIS,
hyyan and
laurenic0l
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
gosteenBASIS
left a comment
gosteenBASIS
left a comment
There was a problem hiding this comment.
As far as I can tell, this all looks good. Clicked through all the pages and checked that the demo viewers all work, and the source code changes look reasonable to me.
I did run into a couple problems when building/running this branch, which I haven't run into before. They were mostly resolved by just cleaning/rebuilding, but I'm noting them here in case they're indicative of any issues, or if the problems occur when we merge this branch:
My first mvn jetty:run didn't work, and warned me that 14 files had "format violations," and it prompted me to run mvn spotless:apply to fix them. I did this, which fixed the problems. These were all Java files in the webforj/samples/views directory. login/LoginBasicView.java, table/renderers/Invoice.java, splitter/SplitterBasicView.java, among others. These files all showed up as "unstaged changes" in my git client, but when I tried to see what the difference was, none of them had any detectable differences. Not sure what any of this is about, or what the format violations were.
I also encountered an issue I hadn't seen before, where the doc site rendered without any styling, with everything in the browser's default font and no layouts or anything. Did a clean, a clear, and a rebuild, and it was fixed.
Possibly these were idiosyncratic to my local environment in some way. In any case, the issues were easily fixed, so I'm approving and leaving the info here for the record.
|
@gosteenBASIS Thanks for the detailed review - that first issue was a plugin Eric was working on to enforce good code styling. That second one is strange, but not something that sounds related to this change. Thanks for looking everything over |
bbrennanbasis
left a comment
bbrennanbasis
left a comment
There was a problem hiding this comment.
-
Now that ComponentDemo has its own folder, and the code is split into three JavaScript files, should the CSS be kept separately, like for AskMenu and ExpandableCode?
-
Possibly a remanent from the original component, but mobile/tablet component demos with a scrollbar don't render ideally.
- Were any of the existing open Issues for ComponentDemo going to be resolved in this PR?
| const GLOBALS = { | ||
| IFRAME_SRC_LIVE: "", | ||
| IFRAME_SRC_DEV: "http://localhost:8080" | ||
| IFRAME_SRC_DEV: "http://localhost:8080", |
There was a problem hiding this comment.
Didn't realize 8080 was still hardcoded in parts of the project, but should be configurable after the changes from PR #754
|
|
||
| async function loadInto(url) { | ||
| try { | ||
| const response = await fetch(url, { signal: controller.signal }); | ||
| if (!response.ok) throw new Error(`${response.status} ${response.statusText}`); | ||
| const text = await response.text(); | ||
| setLoadedFiles((prev) => ({ ...prev, [url]: text })); | ||
| } catch (err) { | ||
| if (err.name === 'AbortError') return; | ||
| console.error(`ComponentDemo: failed to load ${url}`, err); | ||
| setLoadedFiles((prev) => ({ | ||
| ...prev, | ||
| [url]: `// Failed to load ${url}\n// ${err.message}`, | ||
| })); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I had only gotten this error when I ran the build and went to a page with the broken link, was this suppose to validate the links when attempting to check a successfully build?
There was a problem hiding this comment.
Nope, this was because we weren't doing any logging or warning if there was a broken link before, even on deploy, so now there's at least a warning. The warning during development isn't something I tackled this go around.
| urls={['https://raw.githubusercontent.com/webforj/webforj-documentation/refs/heads/main/src/main/java/com/webforj/samples/views/table/MusicRecord.java', | ||
| 'https://raw.githubusercontent.com/webforj/webforj-documentation/refs/heads/main/src/main/java/com/webforj/samples/views/table/Service.java']} | ||
| <ComponentDemo | ||
| path='/webforj/tablemultisorting' |
There was a problem hiding this comment.
[webforJ.Capitalization] Use 'webforJ' instead of 'webforj'.
3 participants
…mproved file passing, fixed bugs and logical errors, and updated markdown files for new props usage