cc-domain-management: init component #1090
Conversation
florian-sanders-cc
force-pushed
the
cc-domain-management/init
branch
4 times, most recently
from
b95dc67
to
4982d85
Compare
|
|
||
| <!-- TODO: --> | ||
| <table> | ||
| <tr><td><strong>Component </strong> <td><a href="https://www.clever-cloud.com/doc/clever-components/?path=/docs/🛠-organisation-cc-orga-member-list--default-story"><code><cc-orga-member-list></code></a> |
There was a problem hiding this comment.
TODO @florian-sanders-cc: once we agree on the component name, edit this link and remove the todo comment.
florian-sanders-cc
added
bug
|
|
||
| new LostFocusController(this, '.mark-primary', () => { | ||
| /** @type {HTMLDivElement|null} */ | ||
| const primaryDomainCard = this.shadowRoot?.querySelector('.primary'); |
There was a problem hiding this comment.
TODO @florian-sanders-cc : use the focusBySelector helper once merged
florian-sanders-cc
force-pushed
the
cc-domain-management/init
branch
2 times, most recently
from
ba5ae98
to
08a7cc0
Compare
florian-sanders-cc
requested review from
hsablonniere,
Galimede,
roberttran-cc,
pdesoyres-cc and
HeleneAmouzou
florian-sanders-cc
force-pushed
the
cc-domain-management/init
branch
9 times, most recently
from
eda0016
to
7e4b9ca
Compare
florian-sanders-cc
force-pushed
the
cc-domain-management/init
branch
5 times, most recently
from
fe5302f
to
6c9dbea
Compare
florian-sanders-cc
force-pushed
the
cc-domain-management/init
branch
from
6c9dbea
to
3c79f30
Compare
There was a problem hiding this comment.
Hey @florian-sanders-cc, well done on this!!! 👏
I created a few comments. I looked at:
- component file
- smart file
- type file
- utils file
And used the different stories in my local Storybook + console
About the UI
As I mentioned in our chat:
- I like the card design better than the list
- Maybe the cards could be more compact with some kind of table layout
- Only when there's one column?
- Long domains have a weird behaviour with the icons
- I would prefer if the domain/prefix/link was on a full width line and then the badges and buttons below
- if you mark a domain as primary, the hover state gets stuck
Naming
- Component naming
- I think the
-managementis good as it's more than just a list
- I think the
- FQDN
- I think we should get rid of the FQDN name. The API no longer returns FQDNs since we introduced the path prefix feature a very long time ago.
- I also think we make as if the API already returns the right details and not just a full string, this means moving everything in the API layer (not in the component and not in the main part of the smart definition)
- For CNAME and A, we should use the word
recordand notvalue
URL/domain manipulation
- We need to consider moving all URL/domain manipulations to a specific helper so we can move it in the client and use if in the CLI
- The component knows about
cleverapps.ioin some translations but it should not know what makes a test only domain as we will add new wildcards in the future
| return 'invalid-wildcard'; | ||
| } | ||
|
|
||
| // TODO: move to `URL.parse()` when support is good enough |
|
|
||
| event.preventDefault(); | ||
|
|
||
| const pasteValue = event.clipboardData.getData('text'); |
There was a problem hiding this comment.
nitpick: pasteValue => clipboardValue (not sure why I don't like pasteValue 😛
| // we trim and remove `https://` / `http` and we send this value to the API but we don't change what is displayed within the input | ||
| const domainValue = this.domainFormState.domain.value | ||
| .trim() | ||
| .replace(/http(s)?:\/\//, '') |
There was a problem hiding this comment.
suggestion: you don't need the capturing group around the s. You can just do this:
.replace(/https?:\/\//, '')Maybe a ^ at the beginning would be
| // we need to decodeURIComponent because "*" is encoded as "%2A" and we don't want that within toast messages for instance | ||
| domainName: decodeURIComponent(hostname), |
There was a problem hiding this comment.
question: I'm confused by this, can you explain it a bit more?
| </cc-badge> | ||
| ` : ''} | ||
|
|
||
| ${domainName.endsWith('cleverapps.io') ? html` |
There was a problem hiding this comment.
issue: you have a isTestOnly but you don't use it.
There was a problem hiding this comment.
Also, the CSS class and i18n translation are named testing-only, I think we should name the property isTestingOnly or update the CSS class and i18n.
src/lib/utils.js
Outdated
| * @return {{ hostname: string, pathname: string }} | ||
| */ | ||
| export function extractDomainParts (domain) { | ||
| const domainWithHttp = domain.match(/^http(s)?:\/\//) != null ? domain : 'https://' + domain; |
There was a problem hiding this comment.
suggestion: same feedback for the useless capturing group of (s).
| @@ -226,6 +226,65 @@ export const translations = { | |||
| //#region cc-doc-list | |||
| 'cc-doc-list.error': `Une erreur est survenue pendant le chargement de la documentation`, | |||
| //#endregion | |||
| //#region cc-domain-management | |||
| 'cc-domain-management.certif.automated': () => sanitize`Que vous utilisiez <code>cleverapps.io</code> ou vos propres noms de domaine avec les applications hébergées par Clever Cloud, un certificat Let's Encrypt est automatiquement généré et renouvelé pour l'accès HTTPS/TLS. Vous n'avez rien à faire, tout est automatisé. Pour les cas spécifiques, reportez vous à la page <a href="https://developers.clever-cloud.com/doc/administrate/ssl/"><span lang="en">Installing TLS Certificates</span> (en Anglais)</a>.`, | |||
There was a problem hiding this comment.
question: I never realized we capitalized Anglais in french, I guess it's a rule?
| 'cc-domain-management.dns.a.desc': () => sanitize`<p>If you choose to use <code>A</code> records, you'll need to update them yourself.</p> | ||
| <p>Keep an eye on our <a href=https://developers.clever-cloud.com/changelog/">changelog</a> or check our <a href="https://developers.clever-cloud.com/api/v4/#load-balancers">v4 API documentation</a> for this.</p>`, | ||
| 'cc-domain-management.dns.a.heading': `A records`, | ||
| 'cc-domain-management.dns.cname.desc': () => sanitize`<p>Using a <code>CNAME</code> record is highly recommended. This automatically keeps your configuration up to date.</p>`, |
There was a problem hiding this comment.
question: we don't mention something like "not for apex domains", maybe it's too much?
| 'cc-domain-management.certif.custom': () => sanitize`Vous pouvez fournir votre propre certificat grâce au <a href="https://api.clever-cloud.com/v2/certificates/new">Gestionnaire de certificats Clever Cloud (en Anglais)</a>.`, | ||
| 'cc-domain-management.certif.heading': `Sécurisez votre application`, | ||
| 'cc-domain-management.dns.a.desc': () => sanitize`<p>Si vous choisissez d'utiliser des enregistrements de type <code>A</code>, vous devrez vous-même assurer leur mise à jour.</p> | ||
| <p>Pensez à surveiller notre <a href="https://developers.clever-cloud.com/changelog/">changelog (en Anglais)</a> ou à utiliser notre <a href="https://developers.clever-cloud.com/api/v4/#load-balancers">API v4 - <span lang="en">Load balancers</span> (en Anglais)</a> pour cela.</p>`, |
There was a problem hiding this comment.
issue: the texts in FR/EN for the links aren't the same
There was a problem hiding this comment.
Very nice component, and the code/state is very clear.
About the UI
- I prefer the card layout.
- Maybe a copy button could be helpful to copy the full domain name
About the code
- I have mosty some suggestions and nitpicks but nothing really blocking
- maybe the component could be named
cc-domains-manager, but I'm ok with the actual naming too.
Once again, this is an awesome work, well done Florian.
src/lib/utils.js
Outdated
| * @param {string} domain | ||
| * @return {boolean} whether the domain is a `cleverapps.io` is HTTP only or not | ||
| */ | ||
| export function isCleverappsDomainHttpOnly (domain) { |
There was a problem hiding this comment.
suggestion: move this function and other 2 below into a dedicated file. This file is more for very generic utilities for resolving lack of the language. I would have put these functions into a domain.js file.
src/lib/utils.js
Outdated
| * @param {string} domain | ||
| * @return {{ hostname: string, pathname: string }} | ||
| */ | ||
| export function extractDomainParts (domain) { |
There was a problem hiding this comment.
suggestion: some unit tests would be nice to understand deeper what this function does.
| 'cc-domain-management.form.path.help': () => sanitize`Par exemple: <code>/api</code> ou <code>/blog</code>`, | ||
| 'cc-domain-management.form.path.label': `Route`, | ||
| 'cc-domain-management.form.submit': `Ajouter le domaine`, | ||
| 'cc-domain-management.form.submit.error': ({ domain }) => `Une erreur est survenue lors de l'ajout du nom de domaine "${domain}"`, |
There was a problem hiding this comment.
nitpick: there is no final dot for this one. We should choose if we put a final dot for error messages or not.
| interface DomainManagementDnsInfoStateLoaded { | ||
| type: 'loaded'; | ||
| cnameValue: string; | ||
| aValues: string[]; |
There was a problem hiding this comment.
suggestion: maybe rename into aRecords
|
|
||
| interface DomainManagementDnsInfoStateLoaded { | ||
| type: 'loaded'; | ||
| cnameValue: string; |
There was a problem hiding this comment.
suggestion: maybe rename into cnameRecord
| .icon=${iconPrimary} | ||
| hide-text | ||
| circle | ||
| @cc-button:click=${() => this._onMarkPrimary(fqdn)} |
There was a problem hiding this comment.
nitpick: you create a function at each render. Maybe you could rely on a data-fqdn attribute instead:
| @cc-button:click=${() => this._onMarkPrimary(fqdn)} | |
| @cc-button:click=${this._onMarkPrimary} | |
| data-fqdn=${fqdn} |
| .icon=${iconDelete} | ||
| hide-text | ||
| circle | ||
| @cc-button:click=${() => this._onDelete(fqdn)} |
There was a problem hiding this comment.
nitpick: you create a function at each render. Maybe you could rely on a data-fqdn attribute instead:
| @cc-button:click=${() => this._onDelete(fqdn)} | |
| @cc-button:click=${this._onDelete} | |
| data-fqdn=${fqdn} |
| const { hostname, pathname } = extractDomainParts(domain.fqdn); | ||
|
|
||
| /** @type {FormattedDomainInfo} */ | ||
| const formattedDomain = { |
There was a problem hiding this comment.
nitpick: you can return directly instead of creating a local variable.
| /** | ||
| * @param {Event & { detail: string }} event | ||
| */ | ||
| _onPathPrefixInput ({ detail: value }) { |
There was a problem hiding this comment.
fix: there is a weird case when you do the following:
- type "/api"
- select all text with ctrl+A
- type Suppr or Delete key
- => you ask for deleting all, but a
/remains
|
|
||
| if (changedProperties.has('domainListState') && this.domainListState.type === 'loaded') { | ||
| this._sortedDomains = this.domainListState.domains | ||
| .map((domain) => { |
There was a problem hiding this comment.
nitpick: rename var into domainState
There was a problem hiding this comment.
Abouy my review
I have checked the code (except for the smart files) and the stories ( english + french). I only have small suggestions and a question. Otherwise, despite it was a lot to review, I thought it was pretty nice and clear. GG ! 👏
About the UI
I prefer the grid option.
Moreover, I think the component name cc-domain-management is good, and I also like the idea of @pdesoyres-cc : cc-domains-manager.
src/translations/translations.fr.js
Outdated
| <p>Contactez notre équipe support pour plus d'informations.</p>`, | ||
| 'cc-domain-management.form.submit.success': ({ domain }) => `Le nom de domaine "${domain}" a bien été associé à votre application.`, | ||
| 'cc-domain-management.form.submit.success-config': ({ domain }) => sanitize`<p>Le nom de domaine "${domain}" a bien été associé à votre application.</p> | ||
| <p>Une configuration manuelle est nécessaire pour faire pointer votre domain vers Clever Cloud. Consultez la section <strong>Configurez vos DNS</strong> pour plus d'informations.</p>`, |
There was a problem hiding this comment.
| <p>Une configuration manuelle est nécessaire pour faire pointer votre domain vers Clever Cloud. Consultez la section <strong>Configurez vos DNS</strong> pour plus d'informations.</p>`, | |
| <p>Une configuration manuelle est nécessaire pour faire pointer votre domaine vers Clever Cloud. Consultez la section <strong>Configurez vos DNS</strong> pour plus d'informations.</p>`, |
src/translations/translations.fr.js
Outdated
| 'cc-domain-management.list.delete.error': ({ domain }) => `Une erreur est survenue lors de la suppression du nom de domaine "${domain}".`, | ||
| 'cc-domain-management.list.delete.success': ({ domain }) => `Le domaine "${domain}" a bien été supprimé.`, | ||
| 'cc-domain-management.list.empty': `Aucun domaine associé à cette application`, | ||
| 'cc-domain-management.list.error-not-found.heading': `Nom de domain introuvable`, |
There was a problem hiding this comment.
| 'cc-domain-management.list.error-not-found.heading': `Nom de domain introuvable`, | |
| 'cc-domain-management.list.error-not-found.heading': `Nom de domaine introuvable`, |
src/translations/translations.fr.js
Outdated
| 'cc-domain-management.list.link.title': ({ domainUrl }) => `Ouvrir - ${domainUrl} - nouvelle fenêtre`, | ||
| 'cc-domain-management.list.loading-error': `Une erreur est survenue pendant le chargement des domaines associés à cette application.`, | ||
| 'cc-domain-management.list.primary.error': ({ domain }) => `Une erreur est survenue lors du passage du nom de domaine "${domain}" en domaine principal.`, | ||
| 'cc-domain-management.list.primary.success': ({ domain }) => `Le domain "${domain}" a bien été défini comme nom de domaine principal.`, |
There was a problem hiding this comment.
| 'cc-domain-management.list.primary.success': ({ domain }) => `Le domain "${domain}" a bien été défini comme nom de domaine principal.`, | |
| 'cc-domain-management.list.primary.success': ({ domain }) => `Le domaine "${domain}" a bien été défini comme nom de domaine principal.`, |
| }; | ||
|
|
||
| export const defaultStory = makeStory(conf, { | ||
| items: [{ |
There was a problem hiding this comment.
question : I am not sure to understand why/how you do not need to put domainFormState in the items for some stories (here for the defaultStory, but also in dataLoadedWithMoreDomains, empty, etc.) and there is still a form in those stories ?
There was a problem hiding this comment.
Hiya, nice work as always! 💪 🤗
Here are some thoughts below:
- Fqdn feels really weird and not self exlanatory, I think either:
- We should write somewhere what that means
- Change the naming
- I prefer the card versions UI
- The name of the component feels self explanatory, I like it
| type: 'adding'; | ||
| domain: { | ||
| value: string; | ||
| error?: null; |
There was a problem hiding this comment.
question: I don't understand why the type is null here?
| type: 'idle'; | ||
| domain: { | ||
| value: string; | ||
| error?: FormError | null; |
There was a problem hiding this comment.
nitpick: as you marked error as an optional property, you can get rid of the | null
|
|
||
| postNewDomain({ apiConfig, ownerId, appId, domainName, pathPrefix }) | ||
| .then(() => { | ||
| if (domainName.endsWith('cleverapps.io')) { |
There was a problem hiding this comment.
nitpick: as it is the same check as istestonly maybe we could put this in a function somewhere. It would allow to easily keep track of the usage and make a change if we need to
| 'cc-domain-management.list.link.title': ({ domainUrl }) => `Open - ${domainUrl} - new window`, | ||
| 'cc-domain-management.list.loading-error': `Something went wrong while loading domains associated to this application.`, | ||
| 'cc-domain-management.list.primary.error': ({ domain }) => `Something went wrong while trying to mark "${domain}" as primary domain.`, | ||
| 'cc-domain-management.list.primary.success': ({ domain }) => `The "${domain}" domain has been successfully marked as primary domain.`, |
There was a problem hiding this comment.
nitpick: domains appears twice in the sentence, it feels a bit redundant.
suggestion:
| 'cc-domain-management.list.primary.success': ({ domain }) => `The "${domain}" domain has been successfully marked as primary domain.`, | |
| 'cc-domain-management.list.primary.success': ({ domain }) => `"${domain}" has been successfully marked as primary domain.`, |
src/translations/translations.en.js
Outdated
| 'cc-domain-management.certif.custom': () => sanitize`You can provide your own certificate by using the <a href="https://api.clever-cloud.com/v2/certificates/new">Clever Cloud Certificate Manager</a>.`, | ||
| 'cc-domain-management.certif.heading': `Secure your application`, | ||
| 'cc-domain-management.dns.a.desc': () => sanitize`<p>If you choose to use <code>A</code> records, you'll need to update them yourself.</p> | ||
| <p>Keep an eye on our <a href=https://developers.clever-cloud.com/changelog/">changelog</a> or check our <a href="https://developers.clever-cloud.com/api/v4/#load-balancers">v4 API documentation</a> for this.</p>`, |
There was a problem hiding this comment.
issue: there's a double quote missing
| <p>Keep an eye on our <a href=https://developers.clever-cloud.com/changelog/">changelog</a> or check our <a href="https://developers.clever-cloud.com/api/v4/#load-balancers">v4 API documentation</a> for this.</p>`, | |
| <p>Keep an eye on our <a href="https://developers.clever-cloud.com/changelog/">changelog</a> or check our <a href="https://developers.clever-cloud.com/api/v4/#load-balancers">v4 API documentation</a> for this.</p>`, |
There was a problem hiding this comment.
Great work on a very nice component! 💯
I've got a few minor feedback.
And about your questions:
- although less consistent, I prefer
cc-domain-managmentas it is closer to what it actually achieves - I don't mind keeping
fqdnbut I see your point: maybe adding a comment can help - grid or cards: I prefer the cards style!
| export function sortDomains (domainA, domainB) { | ||
| const reversedDomainA = domainA.domainName.split('.').reverse().join('.') + domainA.pathPrefix; | ||
| const reversedDomainB = domainB.domainName.split('.').reverse().join('.') + domainB.pathPrefix; | ||
|
|
||
| if (domainA.isPrimary) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (domainB.isPrimary) { | ||
| return 1; | ||
| } | ||
|
|
||
| return reversedDomainA.localeCompare(reversedDomainB); | ||
| } |
There was a problem hiding this comment.
🤓 nitpick:
| export function sortDomains (domainA, domainB) { | |
| const reversedDomainA = domainA.domainName.split('.').reverse().join('.') + domainA.pathPrefix; | |
| const reversedDomainB = domainB.domainName.split('.').reverse().join('.') + domainB.pathPrefix; | |
| if (domainA.isPrimary) { | |
| return -1; | |
| } | |
| if (domainB.isPrimary) { | |
| return 1; | |
| } | |
| return reversedDomainA.localeCompare(reversedDomainB); | |
| } | |
| export function sortDomains (domainA, domainB) { | |
| if (domainA.isPrimary) { | |
| return -1; | |
| } | |
| if (domainB.isPrimary) { | |
| return 1; | |
| } | |
| const reversedDomainA = domainA.domainName.split('.').reverse().join('.') + domainA.pathPrefix; | |
| const reversedDomainB = domainB.domainName.split('.').reverse().join('.') + domainB.pathPrefix; | |
| return reversedDomainA.localeCompare(reversedDomainB); | |
| } |
| import '../cc-button/cc-button.js'; | ||
| import '../cc-block/cc-block.js'; | ||
| import '../cc-block-section/cc-block-section.js'; | ||
| import '../cc-loader/cc-loader.js'; | ||
| import '../cc-badge/cc-badge.js'; | ||
| import '../cc-input-text/cc-input-text.js'; | ||
| import '../cc-notice/cc-notice.js'; |
There was a problem hiding this comment.
🤓 nitpick: should be alphabetical import order.
| * @cssdisplay block | ||
| * | ||
| * @fires {CustomEvent<{ domain: string, pathPrefix: string }>} cc-domain-management:add - Fires when clicking the "Add a domain" button. | ||
| * @fires {CustomEvent<{ fqdn: string }>} cc-domain-management:mark-as-primary - Fires when clicking a "mark as parimary" button. |
There was a problem hiding this comment.
🔨 issue:
| * @fires {CustomEvent<{ fqdn: string }>} cc-domain-management:mark-as-primary - Fires when clicking a "mark as parimary" button. | |
| * @fires {CustomEvent<{ fqdn: string }>} cc-domain-management:mark-as-primary - Fires when clicking a "mark as primary" button. |
| <!-- | ||
| Caution: the primary class is used to focus the primary domain when it changes, do not remove or change this class | ||
| (see LostFocusController within the constructor for more info) | ||
| --> | ||
| <div class="domain ${classMap({ primary: isPrimary, waiting })}" tabindex="-1" > |
There was a problem hiding this comment.
💭 thought: During my QA, when testing the "mark as primary" feature, it worked properly on several tests expect two times where the focus was affected to the wrong card. Unfortunately, I did not managed to get a step-by-step reproducible scenario.
If the issue is well and truly happening randomly, there may be a race condition between the classMap and the LostFocusController.
Let's talk about this synchronously!
👏 praise: Also, good job on the comment that highlight the implementation!
| ` : ''} | ||
|
|
||
| ${isTestingOnly ? html` | ||
| <cc-badge intent="info" weight="outlined" class="testing-only"> |
There was a problem hiding this comment.
🔨 issue: The .testing-only class does not look used anymore.
| @@ -226,6 +226,65 @@ export const translations = { | |||
| //#region cc-doc-list | |||
| 'cc-doc-list.error': `Une erreur est survenue pendant le chargement de la documentation`, | |||
| //#endregion | |||
| //#region cc-domain-management | |||
| 'cc-domain-management.certif.automated': () => sanitize`Que vous utilisiez <code>cleverapps.io</code> ou vos propres noms de domaine avec les applications hébergées par Clever Cloud, un certificat Let's Encrypt est automatiquement généré et renouvelé pour l'accès HTTPS/TLS. Vous n'avez rien à faire, tout est automatisé. Pour les cas spécifiques, reportez vous à la page <a href="https://developers.clever-cloud.com/doc/administrate/ssl/"><span lang="en">Installing TLS Certificates</span> (en Anglais)</a>.`, | |||
There was a problem hiding this comment.
🔨 issue:
| 'cc-domain-management.certif.automated': () => sanitize`Que vous utilisiez <code>cleverapps.io</code> ou vos propres noms de domaine avec les applications hébergées par Clever Cloud, un certificat Let's Encrypt est automatiquement généré et renouvelé pour l'accès HTTPS/TLS. Vous n'avez rien à faire, tout est automatisé. Pour les cas spécifiques, reportez vous à la page <a href="https://developers.clever-cloud.com/doc/administrate/ssl/"><span lang="en">Installing TLS Certificates</span> (en Anglais)</a>.`, | |
| 'cc-domain-management.certif.automated': () => sanitize`Que vous utilisiez <code>cleverapps.io</code> ou vos propres noms de domaine avec les applications hébergées par Clever Cloud, un certificat Let's Encrypt est automatiquement généré et renouvelé pour l'accès HTTPS/TLS. Vous n'avez rien à faire, tout est automatisé. Pour les cas spécifiques, reportez-vous à la page <a href="https://developers.clever-cloud.com/doc/administrate/ssl/"><span lang="en">Installing TLS Certificates</span> (en Anglais)</a>.`, |
| 'cc-domain-management.dns.heading': `Configurez vos DNS`, | ||
| 'cc-domain-management.dns.info.apex': () => sanitize`Pour un domaine APEX ou un sous-domaine disposant déjà d'une zone DNS, référez-vous à notre <a href="https://developers.clever-cloud.com/doc/administrate/domain-names/">documentation (en Anglais)</a>.`, | ||
| 'cc-domain-management.dns.info.heading': `Load balancers dédiés et cas spécifiques`, | ||
| 'cc-domain-management.dns.info.load-balancer': () => sanitize`Si vous bénéficiez d'un <span lang="en">load balancer</span> dédié, référez vous à sa configuration ou contactez le support. Notre équipe pourra également vous aider pour commander un tel service.`, |
There was a problem hiding this comment.
🔨 issue:
| 'cc-domain-management.dns.info.load-balancer': () => sanitize`Si vous bénéficiez d'un <span lang="en">load balancer</span> dédié, référez vous à sa configuration ou contactez le support. Notre équipe pourra également vous aider pour commander un tel service.`, | |
| 'cc-domain-management.dns.info.load-balancer': () => sanitize`Si vous bénéficiez d'un <span lang="en">load balancer</span> dédié, référez-vous à sa configuration ou contactez le support. Notre équipe pourra également vous aider pour commander un tel service.`, |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Fixes #1095
How to review?
A few specific questions
cc-domain-listinstead? It feels more consistent even though it doesn't only deal with the list of domains.fqdnproperty:domain&pathPrefix). Internally, within the component, thepathPrefixcan never benullor empty because it's always at least/.pathPrefixthing does not exist and we have to concatenate (+ encode 👎 ) everything. When no path prefix is set, there is no trailing slash withinfqdn.example.com:{ fqdn: 'example.com' }{ fqdn: 'example.com', domainName: 'example.com', pathPrefix: '/' }fqdnnaming or do we change it? At first I went withdomainWithPathPrefixbut it creates confusion because it's not an actual concatenation ofdomainName + pathPrefix. I opted to keepfqdneven though I don't like the naming because it highlights that this comes from the API. This is some kind of unique identifier we use to communicate with the API but we don't actually use it apart from within the smart.No columns for badges
Issues:
Some people are triggered by the "misaligned" aspect. Note that the more you think about it the more you find it annoying but as a user, there are similar cases that I had never noticed as annoying, for instance in GitHub PR view:
Column dedicated to primary badge
Issues:
Considering "primary" can only applied to one row and that it is always the first row, it feels weird to dedicate a column to it.
Align right for badge columns
Issues:
Badges are pretty far from what they are directly related to: the domain.
My personal preference if you wanna know
Card design
@hsablonniere has suggested a few nice improvements for the card design, feel free to give your opinion about these as well
Issue with domain names wrapping
When there's not enough space (small viewport or long domain name), the domain name wraps to a new line.
Sometimes the link icon ends up alone on a line, sometimes the path prefix ends up on a line.
We cannot really prevent that but we can improve the design so it happens less often by making the domain name span accross the whole width and position the buttons below it:
Gap between cards or not
When there's only one column (small viewport), should we remove the gap between cards and make it look like a "table"?
TODO Flo