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.

Sign up for GitHub

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

Jump to bottom

MM-231 Support config for which tab in sidebar is selected on map startup #236

Merged
merged 8 commits into from
Apr 18, 2024
Merged

MM-231 Support config for which tab in sidebar is selected on map startup #236

merged 8 commits into from
Apr 18, 2024

Conversation

rogup
Copy link
Contributor

@rogup rogup commented Mar 6, 2024

Closes #231

@rogup rogup self-assigned this Mar 6, 2024
@rogup rogup mentioned this pull request Mar 6, 2024
Open
Nick Stokoe and others added 6 commits March 19, 2024 17:19
.prettierignore - tell Prettier not to reformat any of the code.
85921fd 
Prettier is a code linker/formatter with strong opinions and little
configurability, used by some people on this project.
app/model/config-schema.ts - remove trailing semis from class/interfa…
0b9636e 
…ce defs

A purely stylistic change.
app/model/config-schema.ts - correct the desc: fields for some options
480b70d 
Specifically - showAboutPanel and showDirectoryPanel. This fixes a
cut-and-pasta error.
@rogup
Add defaultPanel option to config
a24d370 
Then disable prettier then redo changes to config schema
CONFIG.md - regenerate with npm run generate-config-doc
a78026a
@rogup
Add UT for defaultPanel config
928fd8c
@wu-lee wu-lee force-pushed the 231-default-sidebar-tab branch from 6247039 to 928fd8c Compare March 19, 2024 17:34
@wu-lee
Copy link
Contributor

wu-lee commented Mar 19, 2024 •
edited
Loading

Experimenting with rebase and how it interacts with PRs.

I've:

  • rebased the branch to the top of dev
  • This has resulted in some commits being de-duplicated. (Those to do wit the git CI pipeline added in both, and the MacOS script fix.) In a minor difference between the branch commit f640d0d and the one it reported being cherry-picked from, I infer the main branch one was the correct version (it lacked the npm run build --if-present command), and selected that version.
  • I moved the .prettierignore commit to the top (this is not really part of the PR remit and at least warrants being separated - maybe it can live on master)
  • I merged the two commits altering the config - "Add defaultPanel option to config" and the subsequent one, the first one with Prettier changes, and the second one reverting Prettier changes. This made the actual changes clear.
  • I split out the stylistic changes from "Add defaultPanel option to config" into a prior commit, and followed through - removing all trailing semicolons.
  • I also split the desc: field description corrections into a prior commit.

Pushing this back, beware, the branch will have moved. But this PR has adapted accordingly. Good.

Nick Stokoe added 2 commits March 20, 2024 17:04
app/{model/config-schema.ts,presenter/sidebar.ts} - make defaultPanel…
21df6c7 
… typesafe

Use typescipt's typing to restrict the possible values of defaultPanel
to one of the allowed ones.

Ran into a problem triggering ERR_REQUIRE_ESM with the d3 library,
just by changing the imports... turns out this is a thorny thing to
solve, so avoided it by not importing the things. Instead I needed to
list the allowed panel IDs in the documentation manually, rather than
programatically.

But ESM modules are "the future" so we probably need to convert
Mykomap to support it (and support CommonJS for cases that need that,
probably using Babel transpilation)
CONFIG.md - npm run generate-config-doc
07af693
@wu-lee
Copy link
Contributor

wu-lee commented Mar 20, 2024

I've added some typesafety - seems sensible and idiomatic in this case. Tweaked and regenerated the docs accordingly. Everything appears to work as before, tests pass.

@wu-lee
Copy link
Contributor

wu-lee commented Mar 20, 2024

I think this is ready to merge? If @rogup you are ok with what I've done above?

@wu-lee wu-lee force-pushed the 231-default-sidebar-tab branch from ed6f529 to 07af693 Compare March 29, 2024 19:49
@wu-lee wu-lee force-pushed the 231-default-sidebar-tab branch from 07ec2fa to 07af693 Compare March 29, 2024 21:08
@rogup
Copy link
Contributor Author

rogup commented Apr 1, 2024

@wu-lee thanks for making those changes. I think this is probably ready to merge, once I've tested it on the DotCoop and ICA maps

@rogup rogup merged commit 9939601 into dev Apr 18, 2024
2 of 3 checks passed
@rogup rogup deleted the 231-default-sidebar-tab branch April 19, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rogup rogup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rogup @wu-lee