-
Notifications
You must be signed in to change notification settings - Fork 13
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
Dashboard Redesign #389
base: main
Are you sure you want to change the base?
Dashboard Redesign #389
Conversation
Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/dashboard-redesign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, very nice work! I just have a few suggestions/ideas below.
This will also act as a blocker until we remove the temporary stuff (previously edited products, Hello Test User
, etc).
Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, 6 unresolved discussions (waiting on @IshavSohal)
src/components/home.vue
line 4 at r1 (raw file):
<div> <div class="flex justify-between items-center p-8 border-b border-solid border-black"> <h1 class="text-4xl">RAMP Storylines Editor & Creation Tool</h1>
Translation needed here.
src/components/home.vue
line 5 at r1 (raw file):
<div class="flex justify-between items-center p-8 border-b border-solid border-black"> <h1 class="text-4xl">RAMP Storylines Editor & Creation Tool</h1> <div class="underline">{{ $t('editor.lang.curr') }}</div>
We'll need the language switch to be functional before merging.
src/components/home.vue
line 15 at r1 (raw file):
<div class="mb-5 text-2xl font-semibold">{{ $t('editor.chooseOption') }}</div> <div class="flex justify-center"> <div class="home-btn-container border border-gray-400 border-solid mr-5 create-product">
This is fine as is, but just as a personal preference it feels more aesthetically pleasing to me to have these buttons take up the full width of the container like they've done in the mockups:
Should be as easy as adding the flex-1
class to both this div and the load existing
div just below this one. Open to feedback here!
src/components/home.vue
line 31 at r1 (raw file):
</svg> <span>{{ $t('editor.createProduct') }}</span> <span class="hidden"> {{ $t('editor.createProductHover') }}</span>
We probably don't need the text to change here on hover, it seems a bit jarring. I see that they've done that in the Figma mockup but I'm assuming it's just a mistake. Pretty sure they just wanted to show the hover styling on the button here.
src/components/home.vue
line 135 at r1 (raw file):
</svg> </span> <span>Edit</span>
Translation needed here.
src/lang/lang.csv
line 28 at r1 (raw file):
editor.lang.en,English,1,Anglais,1 editor.lang.fr,French,1,Français,1 editor.lang.curr,English,1,Français,1
Clever use of translations here, but we should probably just use the individual translations for English and French as this could get confusing in the future.
2ef9cb7
to
c67ec8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 5 files reviewed, 6 unresolved discussions (waiting on @RyanCoulsonCA)
src/components/home.vue
line 4 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Translation needed here.
Donethanks
src/components/home.vue
line 5 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
We'll need the language switch to be functional before merging.
Donethanks
src/components/home.vue
line 15 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
This is fine as is, but just as a personal preference it feels more aesthetically pleasing to me to have these buttons take up the full width of the container like they've done in the mockups:
Should be as easy as adding the
flex-1
class to both this div and theload existing
div just below this one. Open to feedback here!
I've made the change and I agree, it definitely looks more appealing.
src/components/home.vue
line 31 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
We probably don't need the text to change here on hover, it seems a bit jarring. I see that they've done that in the Figma mockup but I'm assuming it's just a mistake. Pretty sure they just wanted to show the hover styling on the button here.
That makes sense, I've removed it. I've also included the hover styling on the other buttons
src/components/home.vue
line 135 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Translation needed here.
Donethanks
src/lang/lang.csv
line 28 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Clever use of translations here, but we should probably just use the individual translations for English and French as this could get confusing in the future.
Donethanks, I've also removed it from the lang csv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r1, 2 of 3 files at r2.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @IshavSohal)
src/components/home.vue
line 6 at r2 (raw file):
<h1 class="text-4xl">{{ $t('editor.respectTitle') }}</h1> <router-link :to="{name: 'home', params: { lang: currLang === 'en' ? 'fr' : 'en' }}"> <div class="underline">{{ $t(`editor.lang.${currLang}`) }}</div>
Switching the langauge works well, but the text is currently showing the language that you're currently using instead of the language you're switching to. If you're using English, we want this to say Francais
, and if you're using French we want this to say English.
src/components/home.vue
line 157 at r2 (raw file):
this.currLang = (this.$route.params.lang as string) || 'en'; // just for testing purposes this.userStore.userProfile.userName = 'Test User';
Blocking here until we get this part working (unless we decide to merge it before implementing this on the server side)
c67ec8e
to
98a516e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)
src/components/home.vue
line 6 at r2 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Switching the langauge works well, but the text is currently showing the language that you're currently using instead of the language you're switching to. If you're using English, we want this to say
Francais
, and if you're using French we want this to say English.
Donethanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New UI changes look great 👍
Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)
src/components/home.vue
line 6 at r2 (raw file):
Previously, IshavSohal (Ishav Sohal) wrote…
Donethanks
Only render this link when we are not using the Canada.ca template, otherwise will have two instances of language toggle.
src/components/home.vue
line 19 at r3 (raw file):
<div class="home-btn-container border border-gray-400 border-solid mr-5 home-buttons flex-1"> <router-link :to="{ name: 'metadataNew' }" class="flex justify-center h-full w-full" target> <button class="flex items-center text-xl font-bold">
Using keyboard navigation for the "Create New Storylines Product" and the "Load Existing Storylines Product" requires an additional tab as it focuses the whole container and then focuses the inner label. We would only want to focus one or the other, ideally the entire container.
98a516e
to
a180202
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)
src/components/home.vue
line 6 at r2 (raw file):
Previously, yileifeng (Yi Lei Feng) wrote…
Only render this link when we are not using the Canada.ca template, otherwise will have two instances of language toggle.
Donethanks. I created a template
prop for the home page, and then rendered the link only when its not equal to 'Canada.ca'. Let me know if you have something else in mind.
src/components/home.vue
line 19 at r3 (raw file):
Previously, yileifeng (Yi Lei Feng) wrote…
Using keyboard navigation for the "Create New Storylines Product" and the "Load Existing Storylines Product" requires an additional tab as it focuses the whole container and then focuses the inner label. We would only want to focus one or the other, ideally the entire container.
Good catch, donethanks. Also fixed a similar issue in the back
button for the metadata editor.
a180202
to
443bbd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)
src/components/home.vue
line 6 at r2 (raw file):
Previously, IshavSohal (Ishav Sohal) wrote…
Donethanks. I created a
template
prop for the home page, and then rendered the link only when its not equal to 'Canada.ca'. Let me know if you have something else in mind.
Hmm this strategy will not work since the template gets rendered when we are on the index-ca-en.html or index-ca-fr.html page. For example, following the https://ramp4-pcar4.github.io/storylines-editor/dashboard-redesign/index-ca-en.html#/en/editor demo link will load the Canada template and see the following results with two lang toggles:
Not sure if there is already an existing check in the codebase for this, but a potential solution(?) for this might be to search for "index-ca-en.html" or "index-ca-fr.html" inside the Vue Router object's URL. Another approach would be to set a flag determining if we are using Canada.ca template inside the Router's meta fields and just have the conditional check on that.
9bfd875
to
7ba580d
Compare
Previously, yileifeng (Yi Lei Feng) wrote…
Donethanks, I made it so that the language toggle doesn't render when the current source page is "index-ca-en.html" or "index-ca-fr.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good!
The only issue I noticed was that the page format is completely messed up on mobile:
Reviewed 1 of 5 files at r1, 1 of 3 files at r2, 2 of 2 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)
src/components/home.vue
line 157 at r2 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Blocking here until we get this part working (unless we decide to merge it before implementing this on the server side)
Server side will hopefully be complete next week. Let's hold off for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r2, 2 of 2 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)
5f44afd
to
0089b87
Compare
Donethanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this but we'll keep it open until the server side is finished up.
Reviewed 1 of 3 files at r2, 1 of 1 files at r3, 2 of 2 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @IshavSohal)
0089b87
to
995bdb7
Compare
Related Item(s)
#373
Changes
Testing
Steps:
This change is