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

UI tweaks, script tweak and bug fixes #7

Closed
wants to merge 9 commits into from

Conversation

7Skiez
Copy link

@7Skiez 7Skiez commented May 11, 2021

Great work, Thought it'd be cool to add some UI changes, little bug fixes and script improvements.

7Skiez added 3 commits May 10, 2021 03:49
Set dropdown container width to fit content
Possibility to add multiple classes for each value
@7Skiez 7Skiez force-pushed the ui-tweaks-and-bug-fixes branch from 00912a2 to 4479cfb Compare May 11, 2021 09:14
@7Skiez 7Skiez changed the title UI tweaks and bug fixes UI tweaks, script tweak and bug fixes May 11, 2021
@bilalnurcan47
Copy link

@bobbyiliev bobbyiliev requested a review from tnylea May 19, 2021 10:53
@tnylea
Copy link
Contributor

tnylea commented May 20, 2021

@7Skiez thanks for the PR. The code looks good and I appreciate the fixes; however, it does appear that these UI tweaks have messed up some alignment on the homepage. Here's is quick screencast:

https://www.loom.com/share/6a2d81836ec74ba6ab35a3f77e1199b1

Your branch is the one in the first tab, compare that to the 2nd tab and it looks like there are some alignment issues. If you can get this fixed, I can take another look and then get this merged in.

Thanks again!

Copy link
Contributor

@tnylea tnylea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the last comment in the thread. There are a few alignment issues that this PR has created:

https://www.loom.com/share/6a2d81836ec74ba6ab35a3f77e1199b1

Thanks!

@7Skiez
Copy link
Author

7Skiez commented May 20, 2021

See the last comment in the thread. There are a few alignment issues that this PR has created:

https://www.loom.com/share/6a2d81836ec74ba6ab35a3f77e1199b1

Thanks!

@tnylea Thanks man, Pleasure to be working on the project, Sorry I forgot changing the promo image in the seeders, The whole point of this UI change was to add a floating boat/ship to go with the wave/voyager paradigm (I'm not sure if this could be a new theme or something you could configure through admin). I've added a few more demo ship images in the public theme directory, They're interchangeable through the admin panel.

wave_ships_demo

I changed the width for dropdown containers for this not to happen when the text/content gets smaller :

w-screen_vs_w-max
However if you didn't like the look of it so far, I could just put in the changes for code (excluding UI).

@tnylea
Copy link
Contributor

tnylea commented May 23, 2021

@7Skiez Love those images!

thanks for this PR. I'll be taking a look at it today. Looks super cool! And thanks for the menu UI updates.

@7Skiez
Copy link
Author

7Skiez commented May 24, 2021

This piece of code will help fixing the sequence and auto incrementing problem on PostgreSQL thedevdojo/voyager#3563 after seeding the pgsql database, This will help fixing the unique violation error and allows normal use.

if (config('database.default') === 'pgsql') {
   $tables = \DB::select('SELECT table_name FROM information_schema.tables WHERE table_schema = \'public\' ORDER BY table_name;');
   foreach ($tables as $table) {
      if (\Schema::hasColumn($table->table_name, 'id')) {
         $seq = \DB::table($table->table_name)->max('id') + 1;
         \DB::select('SELECT setval(pg_get_serial_sequence(\'' . $table->table_name . '\', \'id\'), coalesce(' . $seq . ',1), false) FROM ' .$table->table_name);
      }
   }
}

@tnylea
Copy link
Contributor

tnylea commented May 28, 2021

@7Skiez Thanks for these fixes. Loving it!

Before I merge this in, I want to guarantee that it's available to run on PHP7 and PHP8, currently, this PR has a composer.lock file that requires spatie/macroable locked at 2.0 which requires PHP 8.

Here's the error screen that I get when I try and install a fresh copy from your PR branch:

CleanShot 2021-05-27 at 21 43 45@2x

@bobbyiliev and I are working to get some more tests into the system so that way stuff like this can be checked when a PR is opened. I might suggest trying to remove spatie/ray or macroable from your composer.json.

Hope you understand, I want to make sure that as many people who pull the latest version of Wave are able to install it, even if they are on PHP7.

Thanks! Keep me posted with the progress on this. Appreciate all the help.

@7Skiez
Copy link
Author

7Skiez commented May 28, 2021

@tnylea Hi, I got the same error running php 8 on my machine, I'll revert back the lock file and make a new commit.

image

@7Skiez
Copy link
Author

7Skiez commented May 29, 2021

CleanShot 2021-05-27 at 21 43 45@2x

Try running composer install --ignore-platform-reqs instead, It helps with this issue.

@7Skiez
Copy link
Author

7Skiez commented May 29, 2021

@tnylea Please checkout thedevdojo/themes#3 I tried adding a radio button to be able to switch between layouts, and found this bug, Now I need this merged in before I send one last commit.

@7Skiez
Copy link
Author

7Skiez commented May 29, 2021

Added hero layout radio button switch inside admin, theme options.

image

image

@7Skiez 7Skiez force-pushed the ui-tweaks-and-bug-fixes branch from f007ec2 to 2b1d226 Compare May 30, 2021 00:52
Copy link
Collaborator

@bobbyiliev bobbyiliev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to close this PR as it now has quite a bit of merge conflicts.

I believe that you've cherry-picked some of the fixes from here. Thank you for your contribution!

@bobbyiliev bobbyiliev closed this Aug 22, 2022
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.

4 participants