Skip to content

Conversation

@doggu
Copy link
Collaborator

@doggu doggu commented Jan 15, 2024

I noticed that the map was not working and found that sidebar.js was not loading before plotter/*.js, which plotter/main.js needs to use htmlToElement. It seems this is due to sidebar.js and main.js being in separate "content_scripts" entries (which I did...) and the order in which they appear does not indicate the order in which they are evaluated. While I have not been able to reproduce this error since reinstalling (without the fix), the order in which "js" entries are loaded is well-defined, so this PR reverts the change I made (which seems to have solved the issue).

I'm pretty sure I should either not be using htmlToElement in the first place, or the plotter UI should be more integrated into the "Schedule Builder" portion of the extension (and reorganized accordingly).

@doggu doggu added bug Something isn't working browser extension something to do with the Chrome or Firefox extension labels Jan 15, 2024
@doggu doggu self-assigned this Jan 15, 2024
@vercel
Copy link

vercel bot commented Jan 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
gophergrades ✅ Ready (Inspect) Visit Preview Jan 15, 2024 6:27am

@Kanishk-K Kanishk-K requested a review from samyok January 17, 2024 02:19
@doggu
Copy link
Collaborator Author

doggu commented Jan 17, 2024

(I probably should have made this an issue)

I'm thinking of reorganizing the Schedule Builder-related code a bit, basically something like:

chrome-extension
├── plotter
│   ├── apiapi.js
│   ├── plotter.js
│   └── plotter.css
├── sidebar
│   ├── sidebar.css
│   └── sidebar.js
├── sb.js
└── util.js
...

Another reason for this change is that a planned feature was to show popups over building names everywhere in Schedule Builder (rather than just on the Built Schedules page), and making sure the two features cooperate in a larger environment would probably require greater coordination.

If this seems like a good idea I'll proceed with that in this branch (or another if I'm growing this PR too much).

@samyok
Copy link
Owner

samyok commented Jan 17, 2024

@doggu that sounds good, but make it a new branch so I can review and merge this fix separately from new feature work :)

I'm also considering making the extension more lightweight and moving some functionality to the website for easier deploys in the next couple weeks, so I'll also be modifying the extension a fair bit :) Let me know when you're planning on doing that work and creating a PR for it so I can plan around that.

thanks :3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

browser extension something to do with the Chrome or Firefox extension bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants