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.

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

Updated start page to run application #37

Open
wants to merge 3 commits into
base: version-4.0
Choose a base branch
from

Conversation

gaurav-kreeti
Copy link
Contributor

  • created a start page with a start button to start parsing the CSVs
  • change the default page from config to start

UI for succesfully parsing: Screencast from 23-01-24 03:22:31 PM IST.webm

if some error occured:
Screencast from 23-01-24 03:27:15 PM IST.webm

@gaurav-kreeti gaurav-kreeti changed the base branch from develop to version-4.0 January 24, 2024 07:44
@Arp-G
Copy link
Owner

Arp-G commented Jan 25, 2024

Thanks for the PR 🙏 . I will review it soon.

@gaurav-kreeti
Copy link
Contributor Author

Thanks for the PR 🙏 . I will review it soon.

Just a Reminder to review, in case you forgot.

@Arp-G
Copy link
Owner

Arp-G commented Feb 18, 2024

Hi, thanks for the PR again. There are some issues (some of them not directly related to this PR)

I am listing some of them in order of priority

  1. There is some problems with the genstage pipeline sometimes due to which the genstage consumers sit idly and do not make any demand to the producers because of which the csv import just stops. I haven't yet found a solution to this. Just try importing a different number of csvs and you will come across this.

  2. The way configs are working right now is messy, I do not want to keep a CLI app anymore and only want to support interaction with the app via the web interface. So modules like the Csv2sql.Config.Parser are no longer needed. Need to go through the entire config flow and clean it up.

  3. For this new rewrite of the app I am trying to move away from polling the Csv2sql.ProgressTracker module and instead use a pub sub-based system. Where the live view process can subscribe to the progress tracker and the progress tracker process will send progress updates to the live view process. This might not be a good idea since too many progress update messages between the processes can lead to bad performance so polling might be better, but need to give it a try.

  4. Some tests are failing and tests for some critical code paths are missing.

Some other issues include

  • Need to move away from the usage of shorter_maps and remove it
  • Update all deps, there are some warnings when running on latest elixir/erlang, nodejs versions
  • Use latest template live view syntax for the pages, example: case blocks, <%= if .. calls with the template should be avoided. For case blocks just create a helper function, for conditions use the new :if={..} syntax, for loops use the new :for={...} syntax.
  • The UI for start page is very basic right now, its a good start but needs lot of improvement. Like disable updating configs when csv import is in progress, disabling the start import button if entered configs are invalid, etc.

I tried to resolve some of the above issues here but was stuck on the first point. Will look into it again when I get a chance. Meanwhile, you can take a look and continue where I left off. I suggest getting an understanding of the core logic of the app.

@ssinghi ssinghi deleted the UI-change branch September 27, 2024 14:26
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.

2 participants