fix: change open-event to eventyay for small screens #3626

himanshu010 · 2019-11-16T12:38:11Z

Short description of what this resolves:

change open-event to eventyay for small screens in navbar
screenshot :

Checklist

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

iamareebjamal · 2019-11-16T20:37:21Z

app/templates/components/nav-bar.hbs

@@ -48,7 +48,7 @@
      <i class="large content icon"></i>
    </a>
    {{#link-to 'index' class='item' activeClass=''}}
-      <div class="ui header small text">{{config.appName}}</div>
+      <div class="ui header small text">eventyay</div>


Revert this and change eventyay to {{config.appName}} wherever found in the project

Revert this and change eventyay to {{config.appName}} wherever found in the project

Okk i will work on this.
But since we always have to show eventyay there, i think that there is nothing wrong in directly writing eventyay.

since we always have to show eventyay there

Who said that?

This is not eventyay repository, this is open-event-frontend, eventyay is an instance of open-event.

Same difference as Chromium vs Chrome.

okk

This is not eventyay repository, this is open-event-frontend, eventyay is an instance of open-event.

Same difference as Chromium vs Chrome.

okk

Revert this and change eventyay to {{config.appName}} wherever found in the project

Okk i will work on this.
But since we always have to show eventyay there, i think that there is nothing wrong in directly writing eventyay.

We want to show app name here which is entered in admin panel.
Make it settings.appName

himanshu010 · 2019-11-18T08:50:56Z

@kushthedude changed eventyay

iamareebjamal · 2019-11-18T08:53:53Z

app/templates/components/nav-bar.hbs

@@ -48,7 +48,7 @@
      <i class="large content icon"></i>
    </a>
    {{#link-to 'index' class='item' activeClass=''}}
-      <div class="ui header small text">{{config.appName}}</div>


Use config please. Settings is asynchronous so it'll take time to show up

We are using settings for various importing the name and tagline for the appName and various other input we need.
Also setting is globally injected into app as services, So it should not make much of a difference.

Also setting is globally injected into app as services, So it should not make much of a difference.

I don't know any of that. I know that an AJAX request is made for that. This thinking has lead to how slow the app is.

And if settings are meant to be used here, why is there an option of config.appName?

iamareebjamal · 2019-11-18T08:54:43Z

Also, this is not where you need to change anything, You need to replace hardcoded eventyay to config.appName

kushthedude · 2019-11-18T11:24:59Z

I dont know where the config.appName came from as when displaying in wide screen too we are using the setting.appName. That is why we have a name and tagline field defined in settings for this purpose. Not everyone who uses open event as a product will change the config. There are lot of reasons for the slow down of application, that you already have known better than me.

…

On Mon, 18 Nov, 2019, 16:45 Areeb Jamal, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/templates/components/nav-bar.hbs <#3626 (comment)> : > @@ -48,7 +48,7 @@ <i class="large content icon"></i> </a> {{#link-to 'index' class='item' activeClass=''}} - <div class="ui header small text">{{config.appName}}</div> Also setting is globally injected into app as services, So it should not make much of a difference. I don't know any of that. I know that an AJAX request is made for that. This thinking has lead to how slow the app is. And if settings are meant to be used here, why is there an option of config.appName? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3626?email_source=notifications&email_token=AKQMTLSNYFWVSNY4F6753FTQUJ2ONA5CNFSM4JOE74JKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCL4DU6A#discussion_r347324419>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLVQ74VZR43547YZZ2TQUJ2ONANCNFSM4JOE74JA> .

iamareebjamal · 2019-11-18T12:20:58Z

https://github.com/fossasia/open-event-frontend/search?q=config.appName&unscoped_q=config.appName

kushthedude · 2019-11-18T12:26:35Z

https://github.com/fossasia/open-event-frontend/search?q=config.appName&unscoped_q=config.appName

I see. Interesting thing to note is already config.appName is used in mobile row. Why is the open event getting shown in place kf eventyay

himanshu010 · 2019-11-21T12:57:04Z

@iamareebjamal changed config.appname from 'open-event' to eventyay

iamareebjamal · 2019-11-21T13:05:51Z

config/environment.js

@@ -10,7 +10,7 @@ function getSentryServer(dsn, withProtocol = true) {

 module.exports = function(environment) {
  let ENV = {
-    appName                  : process.env.APP_NAME || 'Open Event',
+    appName                  : process.env.APP_NAME || 'eventyay',


Why are you going in completely opposite direction and making it even worse? Read the repository name. eventyay is an instance. Open Event is the project. I don't understand how to make it more clear

#3626 (comment)

Why are you going in completely opposite direction and making it even worse? Read the repository name. eventyay is an instance. Open Event is the project. I don't understand how to make it more clear

@iamareebjamal So you are saying that open event is written there intentionally and you want me to change existing eventyay to config.appname i.e. 'Open Event'

Absolutely!

himanshu010 · 2019-11-26T10:40:11Z

@iamareebjamal
settings.appname is writing 'eventyay' there
changed it to config.appname

kushthedude · 2019-12-05T13:28:20Z

app/templates/components/nav-bar.hbs

@@ -1,7 +1,7 @@
 <div class="mobile hidden row">
  <div class="ui menu">
    {{#link-to 'index' class='header item' activeClass=''}}
-      <h3>{{settings.appName}}</h3>
+      <h3>{{config.appName}}</h3>


Why are you changing Mobile Hidden Row, Change the name in Mobile Row to settings.appName . That's it.
@iamareebjamal Talking about the global injection of settings that can be handled later once this PR is done.

kushthedude · 2019-12-21T16:46:37Z

Stale ?

kushthedude · 2019-12-21T16:47:13Z

Please feel free to reopen when you have addressed the requested change.
Thank You

snitin315 · 2019-12-22T01:51:54Z

@himanshu010 , if you approve I would like to work on this issue.

himanshu010 · 2019-12-22T02:59:07Z

@kushthedude #3715 is my new pull request

auto-label bot added the fix label Nov 16, 2019

iamareebjamal reviewed Nov 16, 2019

View reviewed changes

changing eventyay to {{settings.appName}}

39df8ac

iamareebjamal reviewed Nov 18, 2019

View reviewed changes

changed config.appname to eventyay

7bf244c

iamareebjamal reviewed Nov 21, 2019

View reviewed changes

Changed to config.appname

d21fbbc

kushthedude reviewed Dec 5, 2019

View reviewed changes

kushthedude closed this Dec 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change open-event to eventyay for small screens #3626

fix: change open-event to eventyay for small screens #3626

himanshu010 commented Nov 16, 2019 •

edited

Loading

iamareebjamal Nov 16, 2019 •

edited

Loading

himanshu010 Nov 17, 2019 •

edited

Loading

iamareebjamal Nov 17, 2019

iamareebjamal Nov 17, 2019

himanshu010 Nov 17, 2019

kushthedude Nov 17, 2019

himanshu010 commented Nov 18, 2019

iamareebjamal Nov 18, 2019

kushthedude Nov 18, 2019

iamareebjamal Nov 18, 2019

iamareebjamal commented Nov 18, 2019

kushthedude commented Nov 18, 2019 via email

iamareebjamal commented Nov 18, 2019

kushthedude commented Nov 18, 2019

himanshu010 commented Nov 21, 2019

iamareebjamal Nov 21, 2019

iamareebjamal Nov 21, 2019

himanshu010 Nov 21, 2019

iamareebjamal Nov 21, 2019

himanshu010 commented Nov 26, 2019

kushthedude Dec 5, 2019

kushthedude commented Dec 21, 2019

kushthedude commented Dec 21, 2019

snitin315 commented Dec 22, 2019

himanshu010 commented Dec 22, 2019

fix: change open-event to eventyay for small screens #3626

fix: change open-event to eventyay for small screens #3626

Conversation

himanshu010 commented Nov 16, 2019 • edited Loading

Short description of what this resolves:

Checklist

iamareebjamal Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

himanshu010 Nov 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshu010 commented Nov 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Nov 18, 2019

kushthedude commented Nov 18, 2019 via email

iamareebjamal commented Nov 18, 2019

kushthedude commented Nov 18, 2019

himanshu010 commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshu010 commented Nov 26, 2019

Choose a reason for hiding this comment

kushthedude commented Dec 21, 2019

kushthedude commented Dec 21, 2019

snitin315 commented Dec 22, 2019

himanshu010 commented Dec 22, 2019

himanshu010 commented Nov 16, 2019 •

edited

Loading

iamareebjamal Nov 16, 2019 •

edited

Loading

himanshu010 Nov 17, 2019 •

edited

Loading